From 05ed47256b584e6aeae77698232afc32ed67d704 Mon Sep 17 00:00:00 2001 From: Dev-3 Date: Mon, 2 Nov 2026 04:50:23 +0000 Subject: [PATCH] fix(host): refuse a hard link at the temp name a write lands through lstat reports a hard link as a plain file, so the temp-name guard let a write land in an inode another name holds outside the home (nlink 2). tempWritable now also requires nlink 1: the eight rungs of the sweep (team state, OCR cache, Automation-tasks defaults, Bot Social server choice, workflow store, memory entry and snapshot, decision-log trim) refuse a shared inode, while a crashed host's own plain leftover stays replaceable. --- changelog.d/hardlink-temp-name.fixed.md | 1 + core/src/host/automation-default.js | 2 +- core/src/host/decisions.js | 8 +- core/src/host/images.js | 2 +- core/src/host/memory.js | 10 +- core/src/host/real-dir.js | 16 ++- core/src/host/store.js | 2 +- core/test/automation-default.test.js | 22 +++- core/test/decisions.test.js | 34 +++++- core/test/images.test.js | 20 +++- core/test/memory.test.js | 65 ++++++++++- core/test/real-dir.test.js | 29 ++++- core/test/store.test.js | 21 ++++ docs/subsystems/core-host.md | 4 +- docs/subsystems/social.md | 2 +- docs/subsystems/workflow.md | 2 +- patches/ds-bot/0001-core-fixes.patch | 101 +++++++++++++++--- patches/ds-bot/0002-flybot-api-host.patch | 2 +- patches/ds-bot/0014-flybot-branding.patch | 23 +++- .../files/src/host/automation-default.js | 2 +- patches/ds-bot/files/src/host/decisions.js | 8 +- patches/ds-bot/files/src/host/real-dir.js | 16 ++- .../files/test/automation-default.test.js | 22 +++- patches/ds-bot/files/test/decisions.test.js | 34 +++++- patches/ds-bot/files/test/real-dir.test.js | 29 ++++- plugins/social/src/server-config.js | 10 +- .../social/test/server-config-temp.test.js | 28 ++++- plugins/workflow/src/service.js | 7 +- plugins/workflow/test/temp-name.test.js | 23 +++- 29 files changed, 485 insertions(+), 60 deletions(-) create mode 100644 changelog.d/hardlink-temp-name.fixed.md diff --git a/changelog.d/hardlink-temp-name.fixed.md b/changelog.d/hardlink-temp-name.fixed.md new file mode 100644 index 000000000..8b819ea4e --- /dev/null +++ b/changelog.d/hardlink-temp-name.fixed.md @@ -0,0 +1 @@ +A write that lands through a temporary name now refuses a hard link left there as well as a symbolic one: `lstat` reports a hard link as a plain file, so the team state, the OCR cache, the Automation-tasks defaults, the memory files and snapshot, the decision log's trim and the Bot Social server choice could still be written into an inode another name held outside the Bot's home. The guard asks for a file with only its own name (`nlink === 1`), while the plain leftover a crashed host leaves behind stays replaceable. diff --git a/core/src/host/automation-default.js b/core/src/host/automation-default.js index 1d441947c..a73c2d416 100644 --- a/core/src/host/automation-default.js +++ b/core/src/host/automation-default.js @@ -37,7 +37,7 @@ async function writeOffers(file, offers) { const temp = `${file}.${process.pid}.tmp` // The temp name is a leaf like the defaults file: a link there would take the offer outside // the team folder and the rename would put the link over dsh-defaults.json (temp-name sweep). - if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file, so nothing was written there.`) + if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file of its own, so nothing was written there.`) try { await writeFile(temp, `${JSON.stringify({ version: 1, offers }, null, 2)}\n`, 'utf8') await rename(temp, file) diff --git a/core/src/host/decisions.js b/core/src/host/decisions.js index fe805489a..9e0d6ea17 100644 --- a/core/src/host/decisions.js +++ b/core/src/host/decisions.js @@ -13,7 +13,7 @@ import { dirname, join } from 'node:path' import { cut } from './text.js' import { defineTool } from './tool-schema.js' import { looksSecret } from './memory.js' -import { fileKind } from './real-dir.js' +import { fileKind, tempWritable } from './real-dir.js' import { output } from './tools.js' const OPTION_MAX = 200 @@ -78,11 +78,11 @@ export function install(rt) { const next = kept.length > 0 ? `${kept.join('\n')}\n` : '' if (next !== text) { const tmp = `${path}.tmp` - if (await fileKind(tmp) === 'other') { + if (!tempWritable(tmp)) { trimRefused = true if (!refusedTemp) { refusedTemp = true - rt.warn('ds-bot: the decision log was not trimmed: %s is not a plain file', tmp) + rt.warn('ds-bot: the decision log was not trimmed: %s is not a plain file of its own', tmp) } return kept.length } @@ -162,7 +162,7 @@ export function install(rt) { if (wrote === 'refused') return 'Not recorded: the decision log is not a plain file.' // The feed payload drops the id to a safe shape and adds the live botId (a consumer keys by it). rt.fireFeed?.('decision', { botId, option, choice, ...(why ? { why } : {}), time: entry.time }) - if (wrote === 'untrimmed') return `Written: ${choice} "${option}"${why ? ` — ${why}` : ''} — the log was not trimmed: ${path}.tmp is not a plain file.` + if (wrote === 'untrimmed') return `Written: ${choice} "${option}"${why ? ` — ${why}` : ''} — the log was not trimmed: ${path}.tmp is not a plain file of its own.` return `Recorded: ${choice} "${option}"${why ? ` — ${why}` : ''}.` }, })) diff --git a/core/src/host/images.js b/core/src/host/images.js index 0639f927b..f0ef287c1 100644 --- a/core/src/host/images.js +++ b/core/src/host/images.js @@ -134,7 +134,7 @@ export function install(rt) { const temp = `${ocrPath}.${process.pid}.tmp` // The temp name is a leaf like the cache: a link there would take the readings outside // the home and the rename would put the link over ocr.json (temp-name sweep). - if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file, so the OCR cache was not written there.`) + if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file of its own, so the OCR cache was not written there.`) await writeFile(temp, snapshot, 'utf8') await rename(temp, ocrPath) }).catch(error => rt.warn('ds-bot: OCR cache write failed: %s', error)) diff --git a/core/src/host/memory.js b/core/src/host/memory.js index d8483d910..5acc4a326 100644 --- a/core/src/host/memory.js +++ b/core/src/host/memory.js @@ -32,7 +32,7 @@ import { codePointStart, cut } from '../shared/text-boundary.js' import { output } from './tools.js' import { retiredBot } from './store.js' import { botNode, buildGraph, buildIndex, relatedOf, searchIndex, topicNode } from './memory-search.js' -import { dirKindSync, fileKindSync } from './real-dir.js' +import { dirKindSync, fileKindSync, tempWritable } from './real-dir.js' // Fixed sizes, in characters. Both MEMORY.md files ride in every request of every // part, so together they stay near 8,000 characters, a few thousand tokens. A topic is @@ -453,7 +453,7 @@ export function install(rt) { const warnFile = (path, what = 'memory journal', effect = 'dropping the record') => { if (warnedFiles.has(path)) return warnedFiles.add(path) - rt.warn(`ds-bot: ${what} is not a plain file %s; ${effect}`, path) + rt.warn(`ds-bot: ${what} is not a plain file of its own %s; ${effect}`, path) } // The one answer for "what is this folder": every segment from `memory/` down is asked on its // own, because `lstat` follows a link above the last component — `memory/bots` or a @@ -547,9 +547,9 @@ export function install(rt) { async function writeText(path, text) { await mkdir(dirname(path), { recursive: true }) const temp = `${path}.${process.pid}.tmp` - if (fileKindSync(temp) === 'other') { + if (!tempWritable(temp)) { warnFile(temp, 'the memory write', 'nothing is written there') - throw new Error(`Not saved: ${temp} is not a plain file, so nothing is written there.`) + throw new Error(`Not saved: ${temp} is not a plain file of its own, so nothing is written there.`) } await writeFile(temp, text, 'utf8') await rename(temp, path) @@ -825,7 +825,7 @@ export function install(rt) { // The snapshot's temp name is a leaf like the entry's (#2097): a link left there would // take the frozen memory outside and then replace the snapshot with the link. const temp = `${path}.tmp` - if (fileKindSync(temp) === 'other') warnFile(temp, 'the memory snapshot', 'nothing is written there') + if (!tempWritable(temp)) warnFile(temp, 'the memory snapshot', 'nothing is written there') else { writeFileSync(temp, text, 'utf8') renameSync(temp, path) diff --git a/core/src/host/real-dir.js b/core/src/host/real-dir.js index de82dad09..60c4a123d 100644 --- a/core/src/host/real-dir.js +++ b/core/src/host/real-dir.js @@ -51,9 +51,19 @@ export async function fileKind(path) { // `writeFile` follows a link at the last component, so a link left at `..tmp` would take // the bytes outside the home and the `rename` after them would put the link where the file belongs. // `absent` is a writer's to create and a plain leftover under this host's own pid is replaceable; -// anything else refuses the write before the first byte. The destination itself needs no such -// check: `rename` replaces the name, link included, instead of writing through it. -export const tempWritable = (path) => fileKindSync(path) !== 'other' +// anything else refuses the write before the first byte. A hard link is a plain file to `lstat` +// while its bytes are also a name outside this home (a write lands in the shared inode, not in a +// link), so a name with another name (`nlink > 1`) is refused the same way; a crashed host's own +// leftover is one name and stays replaceable. The destination itself needs no such check: `rename` +// replaces the name, link included, instead of writing through it. +export const tempWritable = (path) => { + try { + const info = lstatSync(path) + return info.isFile() && !info.isSymbolicLink() && info.nlink === 1 + } catch (error) { + return error?.code === 'ENOENT' + } +} export function fileKindSync(path) { try { diff --git a/core/src/host/store.js b/core/src/host/store.js index 6cf111a4a..e53ba802b 100644 --- a/core/src/host/store.js +++ b/core/src/host/store.js @@ -928,7 +928,7 @@ export function install(rt) { const temp = `${statePath}.${process.pid}.tmp` // The temp name is a leaf like the state file itself: a link there would take the team // outside the home and the rename would put the link over state.json (temp-name sweep). - if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file, so the Bot team was not written there.`) + if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file of its own, so the Bot team was not written there.`) try { await mkdir(home, { recursive: true }) const ensureLock = () => { diff --git a/core/test/automation-default.test.js b/core/test/automation-default.test.js index 12c4cdeb4..54daa21f0 100644 --- a/core/test/automation-default.test.js +++ b/core/test/automation-default.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { existsSync, lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { existsSync, linkSync, lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { test } from 'node:test' @@ -161,3 +161,23 @@ test('a linked defaults temp name does not take the offer; the file is not creat assert.equal(existsSync(file) ? lstatSync(file).isSymbolicLink() : false, false, 'the defaults file is not a link') assert.ok(warnings.some(line => /not a plain file/.test(line)), warnings.join('\n')) }) + +// The temp name can be a hard link, which `lstat` reports as a plain file: the offer would land in +// the inode the other name holds. It is refused before the write, once, and the bundle is not +// switched on. +test('a hard link at the defaults temp name keeps the offer unwritten', async (t) => { + const file = scratch(t) + const outside = join(dirname(file), 'outside-defaults.json') + writeFileSync(outside, 'OUTSIDE\n') + linkSync(outside, `${file}.${process.pid}.tmp`) + const warnings = [] + const plugins = manager() + const outcome = await offerBundle({ manager: plugins, profile: 'web', file, warn: (...args) => warnings.push(args.join(' ')) }) + assert.equal(outcome, 'unrecorded') + assert.equal(plugins.on, false, 'the bundle is not switched on') + assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the shared inode keeps its bytes') + assert.equal(lstatSync(outside).nlink, 2, 'the outside file still carries the temp name') + assert.equal(existsSync(file), false, 'the defaults file is not created') + assert.ok(warnings.some(line => /not a plain file/.test(line)), warnings.join('\n')) + assert.ok(warnings.some(line => line.includes(`${file}.${process.pid}.tmp`)), 'the warning names the temp path') +}) diff --git a/core/test/decisions.test.js b/core/test/decisions.test.js index 6ae241478..09e53b4c9 100644 --- a/core/test/decisions.test.js +++ b/core/test/decisions.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { existsSync, lstatSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { existsSync, linkSync, lstatSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -180,3 +180,35 @@ test('#2103: a linked temp name does not take the trim; the log stays a plain fi assert.equal(existsSync(`${plain}.tmp`), false, 'the healthy trim renames its temp name away') assert.deepEqual(healthyWarnings, []) }) + +// The trim's temp name can be a hard link, which `lstat` reports as a plain file: the retained log +// would be written into the inode the other name holds. It is refused before the write, warned +// once, and the log keeps its lines. +test('#P-hardlink: a hard link at the trim temp name does not take the trim', async (t) => { + const line = (n) => `${JSON.stringify({ time: 1_000 + n, bot: 'a', botName: 'Chief', option: `kept ${n}`, choice: 'used' })}\n` + const full = (n) => Array.from({ length: n }, (_, i) => line(i)).join('') + + const { ctx, home, tool, stop } = await boot({ decisions: true }) + t.after(stop) + const elsewhere = mkdtempSync(join(tmpdir(), 'ds-bot-decisions-tmp-target-')) + t.after(() => rmSync(elsewhere, { recursive: true, force: true, maxRetries: 5 })) + const target = join(elsewhere, 'outside.txt') + const before = 'PRE-EXISTING BYTES\n' + writeFileSync(target, before) + const log = join(home, 'decisions.jsonl') + writeFileSync(log, full(2_000)) + linkSync(target, `${log}.tmp`) + + const warnings = [] + ctx.logger.warn = (...args) => warnings.push(args.join(' ')) + const first = await tool('record_decision', { option: 'Trigger the trim', choice: 'used' }) + + assert.equal(readFileSync(target, 'utf8'), before, 'the shared inode keeps its bytes') + assert.equal(lstatSync(target).nlink, 2, 'the outside file still carries the temp name') + const lines = readFileSync(log, 'utf8').split('\n').filter(Boolean) + assert.equal(lines.length, 2_001, 'the refused trim leaves the log as it was, plus the new line') + assert.doesNotMatch(first, /^Recorded:/, 'the answer does not claim a clean record') + assert.equal(warnings.length, 1, 'one warning for the temp name') + assert.match(warnings[0], /decisions\.jsonl\.tmp/) + assert.match(warnings[0], /not a plain file/) +}) diff --git a/core/test/images.test.js b/core/test/images.test.js index cae6e0924..3d4194b95 100644 --- a/core/test/images.test.js +++ b/core/test/images.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { existsSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -185,3 +185,21 @@ test('an ordinary OCR cache temp name is written and renamed as before', async ( assert.equal(warnings.length, 0) assert.equal(JSON.parse(readFileSync(cache, 'utf8'))[OCR_ATTACHMENT.attachmentId].text, 'A red triangle.') }) + +// The temp name can be a hard link: `lstat` reports a plain file, so the link check alone lets the +// cache land in the inode the other name holds. The refusal names the temp path and leaves both +// files as they were. +test('a hard link at the OCR cache temp name does not take the cache', async (t) => { + const { rt, home, warnings } = ocrRuntime(t) + const outside = join(home, 'outside-ocr.txt') + writeFileSync(outside, 'OUTSIDE\n') + linkSync(outside, join(home, `ocr.json.${process.pid}.tmp`)) + await rt.imagesForGroup([OCR_ATTACHMENT], 'r1') + await waitFor(() => warnings.length > 0) + assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the shared inode keeps its bytes') + assert.equal(lstatSync(outside).nlink, 2, 'the outside file still carries the temp name') + const cache = join(home, 'ocr.json') + assert.equal(existsSync(cache), false, 'the cache is not created from the shared file') + assert.ok(warnings.some(line => /not a plain file/.test(line)), warnings.join('\n')) + assert.ok(warnings.some(line => line.includes(`${cache}.${process.pid}.tmp`)), 'the warning names the temp path') +}) diff --git a/core/test/memory.test.js b/core/test/memory.test.js index db0862347..d702b65fa 100644 --- a/core/test/memory.test.js +++ b/core/test/memory.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { hostname, tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { test } from 'node:test' @@ -1724,8 +1724,8 @@ test('#2097: a link left at a writer\'s temp name takes no entry, and the memory t.after(() => { console.error = original }) const { tool, stop } = start({ home }) t.after(() => stop()) - assert.match(await tool('remember', { text: 'Ship on Friday', scope: 'team' }, 'g3'), /is not a plain file, so nothing is written there/) - assert.match(await tool('remember', { text: 'Ship on Monday', scope: 'team' }, 'g3'), /is not a plain file, so nothing is written there/) + assert.match(await tool('remember', { text: 'Ship on Friday', scope: 'team' }, 'g3'), /is not a plain file of its own, so nothing is written there/) + assert.match(await tool('remember', { text: 'Ship on Monday', scope: 'team' }, 'g3'), /is not a plain file of its own, so nothing is written there/) assert.equal(readFileSync(target, 'utf8'), 'PRE-EXISTING LINE\n', 'the file the link names is byte-identical') assert.equal(lstatSync(written).isSymbolicLink(), false, 'the memory file is still a plain file, not the link') assert.equal(readFileSync(written, 'utf8'), '# Memory\n\n- Kept by hand [added: 2026-10-01]\n', 'the memory file kept what it had') @@ -1858,3 +1858,62 @@ test('#2068: a linked summaries folder gets no summary written through it', asyn for (let i = 0; i < 50; i++) await settle() assert.deepEqual(readdirSync(outside), [], 'nothing was written through the link') }) + +// The temp name can be a hard link, which `lstat` reports as a plain file: the entry would land in +// the inode the other name holds and the memory file would then name that shared file. A name that +// is a file but not only this one is refused like a link, and the memory file keeps what it had. +test('#P-hardlink: a hard link at a writer\'s temp name takes no entry', async (t) => { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-memory-')) + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-memory-outside-')) + const target = join(outside, 'taken.md') + writeFileSync(target, 'PRE-EXISTING LINE\n') + const written = join(home, 'memory', 'team', 'MEMORY.md') + mkdirSync(dirname(written), { recursive: true }) + writeFileSync(written, '# Memory\n\n- Kept by hand [added: 2026-10-01]\n') + const temp = `${written}.${process.pid}.tmp` + linkSync(target, temp) + t.after(() => rmSync(outside, { recursive: true, force: true })) + const lines = [] + const original = console.error + console.error = (...args) => { lines.push(args.join(' ')) } + t.after(() => { console.error = original }) + const { tool, stop } = start({ home }) + t.after(() => stop()) + assert.match(await tool('remember', { text: 'Ship on Friday', scope: 'team' }, 'g3'), /is not a plain file of its own, so nothing is written there/) + assert.equal(readFileSync(target, 'utf8'), 'PRE-EXISTING LINE\n', 'the shared inode is byte-identical') + assert.equal(lstatSync(target).nlink, 2, 'the outside file still carries the temp name') + assert.equal(readFileSync(written, 'utf8'), '# Memory\n\n- Kept by hand [added: 2026-10-01]\n', 'the memory file kept what it had') + const warned = lines.filter(line => line.includes('not a plain file')) + assert.equal(warned.length, 1, 'one warning per temp name per process') + assert.ok(warned[0].includes(temp), `the warning names the temp name: ${warned[0]}`) +}) + +// The snapshot's temp name carries the same shape: a hard link there would freeze the memory into +// the shared inode and then put that file where the snapshot belongs. +test('#P-hardlink: a hard link at the snapshot temp name is not written through', async (t) => { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-memory-')) + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-memory-outside-')) + const target = join(outside, 'snapshot.md') + writeFileSync(target, 'PRE-EXISTING LINE\n') + const snapshots = join(home, 'memory', 'snapshots') + mkdirSync(snapshots, { recursive: true }) + const snapshot = join(snapshots, 'b2.md') + const temp = `${snapshot}.tmp` + linkSync(target, temp) + t.after(() => rmSync(outside, { recursive: true, force: true })) + const lines = [] + const original = console.error + console.error = (...args) => { lines.push(args.join(' ')) } + t.after(() => { console.error = original }) + const { section, call, stop } = start({ home }) + t.after(() => stop()) + await call('state', {}) + const text = section('b2') + assert.match(text, /The team memory is shared by every Bot/, 'the section is built even when it cannot be frozen') + assert.equal(readFileSync(target, 'utf8'), 'PRE-EXISTING LINE\n', 'the shared inode is byte-identical') + assert.equal(lstatSync(target).nlink, 2, 'the outside file still carries the temp name') + assert.equal(existsSync(snapshot), false, 'the shared file was not renamed into the snapshot place') + const warned = lines.filter(line => line.includes('not a plain file')) + assert.equal(warned.length, 1, 'said once for the temp name') + assert.ok(warned[0].includes(temp), `the warning names the temp name: ${warned[0]}`) +}) diff --git a/core/test/real-dir.test.js b/core/test/real-dir.test.js index 59687d736..d924a00e0 100644 --- a/core/test/real-dir.test.js +++ b/core/test/real-dir.test.js @@ -3,11 +3,11 @@ // states: a real directory here, nothing there at all, or something else that must not be read // through. `#2056` / `#2057` call the same function instead of writing the check again. import assert from 'node:assert/strict' -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { linkSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' -import { dirKind, dirKindSync, fileKind, fileKindSync } from '../src/host/real-dir.js' +import { dirKind, dirKindSync, fileKind, fileKindSync, tempWritable } from '../src/host/real-dir.js' function bench() { const dir = mkdtempSync(join(tmpdir(), 'ds-bot-real-dir-')) @@ -71,3 +71,28 @@ test('#2079: a leaf file is real, absent, or other, and a link is never a file h assert.equal(fileKindSync(join(dir, 'a-folder')), 'other') } finally { done() } }) + +// The temp name a write lands through is asked in one place (temp-name sweep). A missing name is the +// writer's to create and the plain leftover of this host's own crash is replaceable, so a rejected +// write never leaves a name that locks the file forever; a link, a folder, or a file another name +// also holds (`nlink 2`, which `lstat` reports as a plain file) refuses before the first byte. +test('#P-hardlink: a temp name is writable only when it is absent or a plain file of its own', () => { + const { dir, outside, done } = bench() + try { + const temp = join(dir, 'state.json.123.tmp') + assert.equal(tempWritable(temp), true, 'absent is a writer\'s to create') + writeFileSync(join(dir, 'not-a-dir'), 'x') + assert.equal(tempWritable(join(dir, 'not-a-dir', 'state.json.123.tmp')), false, 'a check that cannot run (ENOTDIR) is not a licence') + writeFileSync(temp, 'left by a crash\n') + assert.equal(tempWritable(temp), true, 'a plain leftover under this host\'s own pid is replaceable') + const shared = join(outside, 'shared.md') + writeFileSync(shared, 'OUTSIDE\n') + linkSync(shared, join(dir, 'shared.json.123.tmp')) + assert.equal(tempWritable(join(dir, 'shared.json.123.tmp')), false, 'a hard link shares its bytes with a name outside this home') + assert.equal(tempWritable(shared), false, 'and the other name is the same inode, both report nlink 2') + symlinkSync(shared, join(dir, 'linked.json.123.tmp')) + assert.equal(tempWritable(join(dir, 'linked.json.123.tmp')), false, 'a link is not a file here') + mkdirSync(join(dir, 'folder.json.123.tmp')) + assert.equal(tempWritable(join(dir, 'folder.json.123.tmp')), false, 'a folder where a file belongs is refused too') + } finally { done() } +}) diff --git a/core/test/store.test.js b/core/test/store.test.js index 6877027c1..1d3360ef0 100644 --- a/core/test/store.test.js +++ b/core/test/store.test.js @@ -355,3 +355,24 @@ test('a link at the state file itself is replaced, not written through', async ( assert.equal(fs.lstatSync(statePath).isSymbolicLink(), false, 'the link is replaced by the saved file') assert.equal(JSON.parse(fs.readFileSync(statePath, 'utf8')).prefs.sweep, 'kept') }) + +// The temp name can also be a hard link: `lstat` reports a plain file, so a link-only check lets it +// through and the save lands in the inode the other name holds (`nlink 2`). A name that is a file +// but not only this one is refused the same way; the leftover plain temp of a crashed host (one +// name, `nlink 1`) stays replaceable, which the next test keeps. +test('a hard link at the state temp name is refused; the save never reaches the shared file', async (t) => { + const { rt, home, statePath, warnings } = runtime(t) + await rt.load() + await rt.save() + const before = fs.readFileSync(statePath, 'utf8') + const outside = join(home, 'outside-state.json') + fs.writeFileSync(outside, 'OUTSIDE\n') + fs.linkSync(outside, `${statePath}.${process.pid}.tmp`) + rt.state.prefs.sweep = 'not saved' + await assert.rejects(rt.save()) + assert.equal(fs.readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the shared inode keeps its bytes') + assert.equal(fs.lstatSync(outside).nlink, 2, 'the outside file still carries the temp name') + assert.equal(fs.readFileSync(statePath, 'utf8'), before, 'the state file kept what it had') + assert.ok(warnings.some(line => /not a plain file/.test(line.join(' '))), 'the refusal is named') + assert.ok(warnings.some(line => line.join(' ').includes(`${statePath}.${process.pid}.tmp`)), 'the warning names the temp path') +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 1117e0142..fddac925e 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -229,7 +229,7 @@ An admin's `@` used to be the room's control plane: every `@Dev #1234 rebase and - `src/host/memory-search.js` derives two recall aids from the memory files that `src/host/memory.js` already reads; both are pure and rebuildable, and the Markdown files stay the only source of truth. - Hybrid ranking: `termsOf`/`buildIndex`/`searchIndex` build a sparse TF-IDF vector per entry over words, word character trigrams (4+ letters, so a shared stem matches across endings) and CJK uni/bi-grams, and rank by cosine. `recall` uses it only when called with `semantic: "true"`; the default exact-word path is unchanged, so an existing call scores and prints exactly as before. - Graph: `buildGraph`/`relatedOf` turn `[[topic]]` references into topic→topic edges and a `by:` metadata value into file→Bot edges; `recall` with `related` walks them (undirected, up to two hops) and lists what is near a topic or Bot. Tests: `core/test/memory-search.test.js` (pure) and the two `recall` cases in `core/test/memory.test.js`. -- The memory folders are entries of their own ([#2057](https://github.com/ThinkFlowLab/FlyBot/issues/2057), [#2068](https://github.com/ThinkFlowLab/FlyBot/issues/2068)): one resolver, `folderKind()` in `src/host/memory.js`, walks every segment from `/memory` down — the root, `memory/bots` for a Bot scope, the scope folder — and answers `real` / `absent` / `other`; `readFolder()` and `writeFolder()` are its two doors. A reader (`scopeDir`, the snapshot read, `savedSummary`, `reviewed.json`) takes only `real`, so a link on any of those segments cannot feed another folder's `MEMORY.md`, topics, frozen prompt snapshot or summary into a prompt, a `recall`, the memory panel or the Project dossier; `absent` and `other` answer what a missing folder answers (the one empty `MEMORY.md` entry and no topics). A host write or unlink (the snapshot, the summary, `reviewed.json`, the archive, the journal — and since #2068 the entry writers behind `remember`/`edit`/`forget`) still creates a folder that is `absent`, as it always did, and refuses `other` instead of writing or deleting through the link; `workflow-store.js` does the same for a workspace's `workflows/` on the read and write side. `other` is named once per folder and process in one warning (`ds-bot: memory folder is not a plain directory …; keeping it empty`, or the `workflow folder` form), because a folder that reads as empty and a folder that cannot be read look alike. The journal's own file is asked as a file too ([#2079](https://github.com/ThinkFlowLab/FlyBot/issues/2079)): `journal.jsonl` must be `absent` (created) or a plain non-link file before a record is appended, and a link, a directory or a check that cannot run drops that one record with a warning once per file and process, while the memory write itself still lands — the journal is a side channel. The temporary name a write lands through is asked the same way ([#2097](https://github.com/ThinkFlowLab/FlyBot/issues/2097)): `..tmp` for an entry write and `.tmp` for a frozen snapshot must be `absent` or a plain non-link file, because `writeFile` follows a link there and the `rename` after it would put the link where the memory file belongs — a link refuses the write with one warning, nothing is renamed into place and the file keeps what it had, while a check that cannot run refuses the same way. The same temp-name guard covers the team state (`state.json..tmp`), the OCR cache (`ocr.json..tmp`) and the Automation-tasks defaults (`dsh-defaults.json..tmp`, refused as `unrecorded` with one warning): a link, a folder or a check that cannot run refuses the write before its first byte, while the destination itself needs no check because `rename` replaces the name, link included. The panel can say which of the two it is looking at ([#2101](https://github.com/ThinkFlowLab/FlyBot/issues/2101)): `memory-view` carries `blocked`, one `{ scope, path }` per scope the host refused (`other` only — `absent` is an ordinary empty home), and the memory panel prints one line naming that folder above its empty title, while an empty or not-yet-created folder renders exactly the copy it rendered before. Out of scope, deliberately: a single `*.md`/`*.json` leaf that is a link is still read, and a local process that can write inside the home can still win a redirect race. +- The memory folders are entries of their own ([#2057](https://github.com/ThinkFlowLab/FlyBot/issues/2057), [#2068](https://github.com/ThinkFlowLab/FlyBot/issues/2068)): one resolver, `folderKind()` in `src/host/memory.js`, walks every segment from `/memory` down — the root, `memory/bots` for a Bot scope, the scope folder — and answers `real` / `absent` / `other`; `readFolder()` and `writeFolder()` are its two doors. A reader (`scopeDir`, the snapshot read, `savedSummary`, `reviewed.json`) takes only `real`, so a link on any of those segments cannot feed another folder's `MEMORY.md`, topics, frozen prompt snapshot or summary into a prompt, a `recall`, the memory panel or the Project dossier; `absent` and `other` answer what a missing folder answers (the one empty `MEMORY.md` entry and no topics). A host write or unlink (the snapshot, the summary, `reviewed.json`, the archive, the journal — and since #2068 the entry writers behind `remember`/`edit`/`forget`) still creates a folder that is `absent`, as it always did, and refuses `other` instead of writing or deleting through the link; `workflow-store.js` does the same for a workspace's `workflows/` on the read and write side. `other` is named once per folder and process in one warning (`ds-bot: memory folder is not a plain directory …; keeping it empty`, or the `workflow folder` form), because a folder that reads as empty and a folder that cannot be read look alike. The journal's own file is asked as a file too ([#2079](https://github.com/ThinkFlowLab/FlyBot/issues/2079)): `journal.jsonl` must be `absent` (created) or a plain non-link file before a record is appended, and a link, a directory or a check that cannot run drops that one record with a warning once per file and process, while the memory write itself still lands — the journal is a side channel. The temporary name a write lands through is asked the same way ([#2097](https://github.com/ThinkFlowLab/FlyBot/issues/2097)): `..tmp` for an entry write and `.tmp` for a frozen snapshot must be `absent` or a plain non-link file, because `writeFile` follows a link there and the `rename` after it would put the link where the memory file belongs — a symbolic or hard link refuses the write with one warning, nothing is renamed into place and the file keeps what it had, while a check that cannot run refuses the same way. A hard link is a plain file to `lstat` although its bytes are also a name outside this home, so the guard (`tempWritable()` in `src/host/real-dir.js`) also requires the file to carry only this one name (`nlink === 1`): a shared inode is refused exactly like a link, and the plain temp a crashed host leaves behind keeps its single name and stays replaceable. The same temp-name guard covers the team state (`state.json..tmp`), the OCR cache (`ocr.json..tmp`) and the Automation-tasks defaults (`dsh-defaults.json..tmp`, refused as `unrecorded` with one warning): a symbolic link, a hard link, a folder or a check that cannot run refuses the write before its first byte, while the destination itself needs no check because `rename` replaces the name, link included. The panel can say which of the two it is looking at ([#2101](https://github.com/ThinkFlowLab/FlyBot/issues/2101)): `memory-view` carries `blocked`, one `{ scope, path }` per scope the host refused (`other` only — `absent` is an ordinary empty home), and the memory panel prints one line naming that folder above its empty title, while an empty or not-yet-created folder renders exactly the copy it rendered before. Out of scope, deliberately: a single `*.md`/`*.json` leaf that is a link is still read, and a local process that can write inside the home can still win a redirect race. - `recall` shows at most 30 entries (`RECALL_MAX` in `src/host/memory.js`) and now says when it cut the list ([#2001](https://github.com/ThinkFlowLab/FlyBot/issues/2001)): the exact-word path prints `N more entries match; narrow the query or read a topic` with the real count it already holds, and `semantic: "true"` asks `searchIndex` for one past the cap and prints `more entries are close in meaning than are shown; narrow the query or read a topic` because that index returns the top k only. The cap stays 30; a reply that shows every match adds neither line. ### Plugin memory seam (#450 item 1) @@ -238,7 +238,7 @@ An admin's `@` used to be the room's control plane: every `@Dev #1234 rebase and ### Decisions a Bot records (#396) -- `src/host/decisions.js` gives a Bot one small tool, `record_decision({ option, choice, why })` (`choice` is `used` or `rejected`), so a "considered and rejected" choice a reviewer cannot otherwise see is written down. The host appends one JSON line per decision to `/decisions.jsonl` (bounded on disk at the newest 2000 lines and 1 MB, trimmed by the host as it appends; a read parses at most 1 MB; one write at a time; a write failure is a warning, never a turn failure) and fires the entry on the activity feed as `decision`. A Bot has no tool to edit or delete a past entry. The log is one leaf file beside the home, so a write asks the leaf first ([#2089](https://github.com/ThinkFlowLab/FlyBot/issues/2089)): when `decisions.jsonl` is a link — or anything other than a plain file — the line is dropped, the refusal is one warning, and the tool answers `Not recorded: the decision log is not a plain file.` instead of reporting a line that is not on disk, while an absent file is still created. The temp name the housekeeping lands through is asked the same way ([#2103](https://github.com/ThinkFlowLab/FlyBot/issues/2103)): `/decisions.jsonl.tmp` is written and then renamed over the log, so when it is a link — or anything other than a plain file — the trim is refused instead of carrying the retained log out through it and putting the link where the log belongs; the line just appended stays, the log simply grows past its bound, the refusal is one warning, and the tool answers `Written: … — the log was not trimmed: …` rather than `Recorded:`. A normal temp name trims exactly as before. +- `src/host/decisions.js` gives a Bot one small tool, `record_decision({ option, choice, why })` (`choice` is `used` or `rejected`), so a "considered and rejected" choice a reviewer cannot otherwise see is written down. The host appends one JSON line per decision to `/decisions.jsonl` (bounded on disk at the newest 2000 lines and 1 MB, trimmed by the host as it appends; a read parses at most 1 MB; one write at a time; a write failure is a warning, never a turn failure) and fires the entry on the activity feed as `decision`. A Bot has no tool to edit or delete a past entry. The log is one leaf file beside the home, so a write asks the leaf first ([#2089](https://github.com/ThinkFlowLab/FlyBot/issues/2089)): when `decisions.jsonl` is a link — or anything other than a plain file — the line is dropped, the refusal is one warning, and the tool answers `Not recorded: the decision log is not a plain file.` instead of reporting a line that is not on disk, while an absent file is still created. The temp name the housekeeping lands through is asked the same way ([#2103](https://github.com/ThinkFlowLab/FlyBot/issues/2103)): `/decisions.jsonl.tmp` is written and then renamed over the log, so when it is a link, a hard link (`nlink > 1`, whose bytes another name also holds) or anything other than a plain file of its own, the trim is refused instead of carrying the retained log out through it and putting the link where the log belongs; the line just appended stays, the log simply grows past its bound, the refusal is one warning, and the tool answers `Written: … — the log was not trimmed: …` rather than `Recorded:`. A normal temp name trims exactly as before. - **Opt-in** (`config.decisions`): with the flag off the tool is not registered, nothing is written and no event is fired, so a default install is unchanged. A key, token or password in the text is refused (`looksSecret`), so the log needs no secret. Tests: `core/test/decisions.test.js`. ### Tasks diff --git a/docs/subsystems/social.md b/docs/subsystems/social.md index 50271fd0d..757a30790 100644 --- a/docs/subsystems/social.md +++ b/docs/subsystems/social.md @@ -151,7 +151,7 @@ Creating a group opens its Session with zero turns, and the host draws no sessio ### The server in Settings (#337) -- **Where it comes from.** `config.server` in the profile patch is the default. A server picked in Settings → **Bot Social** is kept in `flybot-social-server.json` (mode 600, next to the credentials file) and wins while it exists; choosing the default again removes the file. It is written through `flybot-social-server.json..tmp`, and that temp name is asked as a file first (temp-name sweep): a link left there refuses the save instead of putting the choice outside the folder and renaming the link into place. Nothing configured anywhere behaves as before (the start fails with `server URL is required`, `Social is not running: …`; the page says **No server set**). The page reads `social/server` (a read; `{ server, source: settings|config|none, defaultServer, state: connected|connecting|not-configured|error, handle, error, canChange, known[], platform? }`: never a token) and calls `social/server-check` (it makes the host fetch an address the caller names, so it follows the write rules like `social/server-set`). +- **Where it comes from.** `config.server` in the profile patch is the default. A server picked in Settings → **Bot Social** is kept in `flybot-social-server.json` (mode 600, next to the credentials file) and wins while it exists; choosing the default again removes the file. It is written through `flybot-social-server.json..tmp`, and that temp name is asked as a file first (temp-name sweep): a symbolic or hard link left there refuses the save instead of putting the choice outside the folder and renaming the link into place — `lstat` reports a hard link as a plain file while its bytes belong to another name too, so the guard also requires `nlink === 1`, and the plain leftover of a crashed host stays replaceable. Nothing configured anywhere behaves as before (the start fails with `server URL is required`, `Social is not running: …`; the page says **No server set**). The page reads `social/server` (a read; `{ server, source: settings|config|none, defaultServer, state: connected|connecting|not-configured|error, handle, error, canChange, known[], platform? }`: never a token) and calls `social/server-check` (it makes the host fetch an address the caller names, so it follows the write rules like `social/server-set`). - **Validation.** `parseServerUrl`: `https://` only, `http://` just for `localhost`, `127.0.0.1` and `[::1]`; no user name or password, query or fragment; at most 200 characters; an address without a scheme gets `https://` (`http://` for those local hosts). `probeServer` GETs `/v1/platform` without credentials, 8 s deadline, `redirect: 'manual'` (a redirect is refused, so the saved address is the checked one); the answer must look like Bot Social (a `name`, a `platform_key` or a `features` list) and its `features` are shown (cleaned, at most 40) in words (`featureLine` in `client.js`: `typed_by_owner` → typing as you, `mentions` → @mentions, `presence` → online status; an unknown id is only counted, and every raw id is in the line's tooltip, #421). `social/server-set` repeats both before it touches anything, so a UI that skipped **Check** gains nothing. - **A server is an identity.** The default server keeps its files. Any other server gets `flybot-social.json.` (credentials; the same suffix `startSocial` already used for backups, `tag` = first 8 hex of the sha-256 of the normalized address) and `flybot-social-settings..json`; a different server never reads another's file, and no credentials file is ever deleted or rewritten by a switch. The per-Bot parts that live in host state (the `member` identity, the `autoApprove` marks and the person/room grants, which are Bot Social ids and handles) are parked in `flybot-social-bots..json` (no token) and cleared before the new server starts, so a stale grant to `@bob.home` can never apply to a different Bob on the new server; switching back restores them and the saved credentials, without a second onboarding. The old server's friends' Bots leave the list when the new server's first roster fetch replaces it (`remote.publish`); face-to-face rooms already hosted stay as local transcripts, but nothing polls them. - **Applied live, with a way back.** `social/server-set` (needs `confirm: true`; a write, so a read-only host and a multi-member team refuse it) stops the social layer, parks the old server's Bot identities, saves the choice, restores what the new server had, and starts again (first start onboards). If that start fails or takes over 45 s, the previous server's files, identities and choice are put back, it is started again, and the error says so; a start that finishes late is disposed. A disposed instance never re-arms its poll: a face poll already in flight when `dispose()` runs cannot install a new interval on the discarded layer, so it never keeps polling (or delivering from) the server it was built for. When the switch succeeds and the address really changed, the previous server's hosted face-to-face rooms — open or still forming (candidate) — are marked `left` here — a read-only past room whose composer is gone, since the new server cannot reach them. Renaming this home on the same server is not a switch: that server still reaches those rooms, so only a new address ends them (the room and its composer stay as they were). Switching back does not reopen them: a room this home left stays a past room (`faceSnapshotPast`, #182), and a line to it is refused (`bot/room-ended`). The Settings page says plainly that this is a new identity, that friends and sharing stay on the old server, and that the old sign-in is kept as a backup and never deleted. Tests: `plugins/social/test/social.test.js` (`Settings server`, `server controller`, `parseServerUrl`, `probeServer`). diff --git a/docs/subsystems/workflow.md b/docs/subsystems/workflow.md index 1b6221ad1..816d8c0a4 100644 --- a/docs/subsystems/workflow.md +++ b/docs/subsystems/workflow.md @@ -40,7 +40,7 @@ Everything in `plugins/workflow/`: the graph rules, the run store, the matcher, ## Entry points - Endpoint `workflow/state` and `workflow/templates` are reads. -- `workflow/create`, `workflow/rename`, `workflow/delete`, `workflow/assign`, `workflow/concurrency`, `workflow/graph`, `workflow/advance`, `workflow/reset`, `workflow/ingest` write. `workflow/concurrency` sets that workflow's `maxConcurrent` (an integer from 1 to 8); the same control is in Settings → Workflow graphs. `workflow/step-timeout` sets that workflow's `stepTimeoutMinutes` (an integer from 1 to 120, or `null` to go back to the default), and `workflow/create` and `define_workflow` accept the same field, refused outside 1..120 rather than dropped (#1868). Every write lands through `workflows.json..tmp`, and that temp name is asked as a file first (temp-name sweep): a link left there refuses the write, with the temp name in the answer, instead of putting the workflows outside their folder and renaming the link into place. `workflow/rename` updates a workflow's name and optional description; read-only hosts are refused. `workflow/delete` removes a workflow from state, releases its ephemeral Bots if a run exists, and cleans logs/runs/routes and every task claim that named it, so the next `ingest` of that task matches again instead of asking `assign` for a workflow that no longer exists; a load still drops a workflow claim whose workflow is gone, so a store written before this fix heals (#1632). `workflow/ingest` also takes `rematch: true`: that one task's claim and route are dropped before matching, the panel's per-task action (#1647). `workflow/assign` reads `taskId` (or `id`), `title`, `brief`, `callerBotId`, `projectId` and `botId`; any other key is named in the response's `ignored` array instead of being dropped in silence (#1869). +- `workflow/create`, `workflow/rename`, `workflow/delete`, `workflow/assign`, `workflow/concurrency`, `workflow/graph`, `workflow/advance`, `workflow/reset`, `workflow/ingest` write. `workflow/concurrency` sets that workflow's `maxConcurrent` (an integer from 1 to 8); the same control is in Settings → Workflow graphs. `workflow/step-timeout` sets that workflow's `stepTimeoutMinutes` (an integer from 1 to 120, or `null` to go back to the default), and `workflow/create` and `define_workflow` accept the same field, refused outside 1..120 rather than dropped (#1868). Every write lands through `workflows.json..tmp`, and that temp name is asked as a file first (temp-name sweep): a symbolic or hard link left there refuses the write, with the temp name in the answer, instead of putting the workflows outside their folder and renaming the link into place — `lstat` reports a hard link as a plain file although another name holds the same bytes, so the guard also requires `nlink === 1`, while a crashed host's own plain leftover stays replaceable. `workflow/rename` updates a workflow's name and optional description; read-only hosts are refused. `workflow/delete` removes a workflow from state, releases its ephemeral Bots if a run exists, and cleans logs/runs/routes and every task claim that named it, so the next `ingest` of that task matches again instead of asking `assign` for a workflow that no longer exists; a load still drops a workflow claim whose workflow is gone, so a store written before this fix heals (#1632). `workflow/ingest` also takes `rematch: true`: that one task's claim and route are dropped before matching, the panel's per-task action (#1647). `workflow/assign` reads `taskId` (or `id`), `title`, `brief`, `callerBotId`, `projectId` and `botId`; any other key is named in the response's `ignored` array instead of being dropped in silence (#1869). - Tool `run_workflow` starts a named run with a brief. Tool `define_workflow` writes the same graph the canvas edits, and takes the same `stepTimeoutMinutes` (1..120) so a lane created that way does not silently inherit the 45-minute default (#1868). - `flybot.tasks.onWrite` calls `ingest`. A new Bot match returns `{ ownerBotId }`. A Workflow match starts the run inside the plugin and leaves the card's owner to the host. A host that offers no `tasks.onWrite` warns once and reports `tasksWired: false` in `workflow/state`, and the Workflows panel says that a board card cannot start a run there (#1462). diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index dc23d2c3e..cc446de45 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -4930,7 +4930,7 @@ diff --git a/src/host/images.js b/src/host/images.js const temp = `${ocrPath}.${process.pid}.tmp` + // The temp name is a leaf like the cache: a link there would take the readings outside + // the home and the rename would put the link over ocr.json (temp-name sweep). -+ if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file, so the OCR cache was not written there.`) ++ if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file of its own, so the OCR cache was not written there.`) await writeFile(temp, snapshot, 'utf8') await rename(temp, ocrPath) }).catch(error => rt.warn('ds-bot: OCR cache write failed: %s', error)) @@ -5011,7 +5011,7 @@ diff --git a/src/host/memory.js b/src/host/memory.js import { output } from './tools.js' +import { retiredBot } from './store.js' +import { botNode, buildGraph, buildIndex, relatedOf, searchIndex, topicNode } from './memory-search.js' -+import { dirKindSync, fileKindSync } from './real-dir.js' ++import { dirKindSync, fileKindSync, tempWritable } from './real-dir.js' // Fixed sizes, in characters. Both MEMORY.md files ride in every request of every // part, so together they stay near 8,000 characters, a few thousand tokens. A topic is @@ -5082,7 +5082,7 @@ diff --git a/src/host/memory.js b/src/host/memory.js + const warnFile = (path, what = 'memory journal', effect = 'dropping the record') => { + if (warnedFiles.has(path)) return + warnedFiles.add(path) -+ rt.warn(`ds-bot: ${what} is not a plain file %s; ${effect}`, path) ++ rt.warn(`ds-bot: ${what} is not a plain file of its own %s; ${effect}`, path) + } + // The one answer for "what is this folder": every segment from `memory/` down is asked on its + // own, because `lstat` follows a link above the last component — `memory/bots` or a @@ -5179,9 +5179,9 @@ diff --git a/src/host/memory.js b/src/host/memory.js async function writeText(path, text) { await mkdir(dirname(path), { recursive: true }) const temp = `${path}.${process.pid}.tmp` -+ if (fileKindSync(temp) === 'other') { ++ if (!tempWritable(temp)) { + warnFile(temp, 'the memory write', 'nothing is written there') -+ throw new Error(`Not saved: ${temp} is not a plain file, so nothing is written there.`) ++ throw new Error(`Not saved: ${temp} is not a plain file of its own, so nothing is written there.`) + } await writeFile(temp, text, 'utf8') await rename(temp, path) @@ -5344,7 +5344,7 @@ diff --git a/src/host/memory.js b/src/host/memory.js + // The snapshot's temp name is a leaf like the entry's (#2097): a link left there would + // take the frozen memory outside and then replace the snapshot with the link. + const temp = `${path}.tmp` -+ if (fileKindSync(temp) === 'other') warnFile(temp, 'the memory snapshot', 'nothing is written there') ++ if (!tempWritable(temp)) warnFile(temp, 'the memory snapshot', 'nothing is written there') + else { + writeFileSync(temp, text, 'utf8') + renameSync(temp, path) @@ -10789,7 +10789,7 @@ diff --git a/test/images.test.js b/test/images.test.js +++ b/test/images.test.js @@ -1,6 +1,10 @@ import assert from 'node:assert/strict' -+import { existsSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' ++import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import { test } from 'node:test' @@ -10799,7 +10799,7 @@ diff --git a/test/images.test.js b/test/images.test.js test('offloadTargets selects live images from prior user and tool messages', () => { const events = [ -@@ -32,3 +32,152 @@ test('offloadTargets selects live images from prior user and tool messages', () +@@ -32,3 +32,170 @@ test('offloadTargets selects live images from prior user and tool messages', () { seq: 2, imageIndexes: [0] }, ]) }) @@ -10952,6 +10952,24 @@ diff --git a/test/images.test.js b/test/images.test.js + assert.equal(warnings.length, 0) + assert.equal(JSON.parse(readFileSync(cache, 'utf8'))[OCR_ATTACHMENT.attachmentId].text, 'A red triangle.') +}) ++ ++// The temp name can be a hard link: `lstat` reports a plain file, so the link check alone lets the ++// cache land in the inode the other name holds. The refusal names the temp path and leaves both ++// files as they were. ++test('a hard link at the OCR cache temp name does not take the cache', async (t) => { ++ const { rt, home, warnings } = ocrRuntime(t) ++ const outside = join(home, 'outside-ocr.txt') ++ writeFileSync(outside, 'OUTSIDE\n') ++ linkSync(outside, join(home, `ocr.json.${process.pid}.tmp`)) ++ await rt.imagesForGroup([OCR_ATTACHMENT], 'r1') ++ await waitFor(() => warnings.length > 0) ++ assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the shared inode keeps its bytes') ++ assert.equal(lstatSync(outside).nlink, 2, 'the outside file still carries the temp name') ++ const cache = join(home, 'ocr.json') ++ assert.equal(existsSync(cache), false, 'the cache is not created from the shared file') ++ assert.ok(warnings.some(line => /not a plain file/.test(line)), warnings.join('\n')) ++ assert.ok(warnings.some(line => line.includes(`${cache}.${process.pid}.tmp`)), 'the warning names the temp path') ++}) diff --git a/test/install.test.js b/test/install.test.js --- a/test/install.test.js +++ b/test/install.test.js @@ -11338,7 +11356,7 @@ diff --git a/test/memory.test.js b/test/memory.test.js @@ -1,7 +1,7 @@ import assert from 'node:assert/strict' -import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' -+import { existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' ++import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { hostname, tmpdir } from 'node:os' -import { join } from 'node:path' +import { dirname, join } from 'node:path' @@ -11577,7 +11595,7 @@ diff --git a/test/memory.test.js b/test/memory.test.js const view = (await call('memory-view', { botId: 'b2' })).value assert.equal(view.error, null) assert.equal(view.summary?.text, 'You like tea.') -@@ -1076,3 +1076,702 @@ test('a memory call names the current part of the Bot chat', async (t) => { +@@ -1076,3 +1076,761 @@ test('a memory call names the current part of the Bot chat', async (t) => { assert.equal(answer.ok, true) assert.equal(requests[0].sessionId, 'b2-part2') }) @@ -12146,8 +12164,8 @@ diff --git a/test/memory.test.js b/test/memory.test.js + t.after(() => { console.error = original }) + const { tool, stop } = start({ home }) + t.after(() => stop()) -+ assert.match(await tool('remember', { text: 'Ship on Friday', scope: 'team' }, 'g3'), /is not a plain file, so nothing is written there/) -+ assert.match(await tool('remember', { text: 'Ship on Monday', scope: 'team' }, 'g3'), /is not a plain file, so nothing is written there/) ++ assert.match(await tool('remember', { text: 'Ship on Friday', scope: 'team' }, 'g3'), /is not a plain file of its own, so nothing is written there/) ++ assert.match(await tool('remember', { text: 'Ship on Monday', scope: 'team' }, 'g3'), /is not a plain file of its own, so nothing is written there/) + assert.equal(readFileSync(target, 'utf8'), 'PRE-EXISTING LINE\n', 'the file the link names is byte-identical') + assert.equal(lstatSync(written).isSymbolicLink(), false, 'the memory file is still a plain file, not the link') + assert.equal(readFileSync(written, 'utf8'), '# Memory\n\n- Kept by hand [added: 2026-10-01]\n', 'the memory file kept what it had') @@ -12280,6 +12298,65 @@ diff --git a/test/memory.test.js b/test/memory.test.js + for (let i = 0; i < 50; i++) await settle() + assert.deepEqual(readdirSync(outside), [], 'nothing was written through the link') +}) ++ ++// The temp name can be a hard link, which `lstat` reports as a plain file: the entry would land in ++// the inode the other name holds and the memory file would then name that shared file. A name that ++// is a file but not only this one is refused like a link, and the memory file keeps what it had. ++test('#P-hardlink: a hard link at a writer\'s temp name takes no entry', async (t) => { ++ const home = mkdtempSync(join(tmpdir(), 'ds-bot-memory-')) ++ const outside = mkdtempSync(join(tmpdir(), 'ds-bot-memory-outside-')) ++ const target = join(outside, 'taken.md') ++ writeFileSync(target, 'PRE-EXISTING LINE\n') ++ const written = join(home, 'memory', 'team', 'MEMORY.md') ++ mkdirSync(dirname(written), { recursive: true }) ++ writeFileSync(written, '# Memory\n\n- Kept by hand [added: 2026-10-01]\n') ++ const temp = `${written}.${process.pid}.tmp` ++ linkSync(target, temp) ++ t.after(() => rmSync(outside, { recursive: true, force: true })) ++ const lines = [] ++ const original = console.error ++ console.error = (...args) => { lines.push(args.join(' ')) } ++ t.after(() => { console.error = original }) ++ const { tool, stop } = start({ home }) ++ t.after(() => stop()) ++ assert.match(await tool('remember', { text: 'Ship on Friday', scope: 'team' }, 'g3'), /is not a plain file of its own, so nothing is written there/) ++ assert.equal(readFileSync(target, 'utf8'), 'PRE-EXISTING LINE\n', 'the shared inode is byte-identical') ++ assert.equal(lstatSync(target).nlink, 2, 'the outside file still carries the temp name') ++ assert.equal(readFileSync(written, 'utf8'), '# Memory\n\n- Kept by hand [added: 2026-10-01]\n', 'the memory file kept what it had') ++ const warned = lines.filter(line => line.includes('not a plain file')) ++ assert.equal(warned.length, 1, 'one warning per temp name per process') ++ assert.ok(warned[0].includes(temp), `the warning names the temp name: ${warned[0]}`) ++}) ++ ++// The snapshot's temp name carries the same shape: a hard link there would freeze the memory into ++// the shared inode and then put that file where the snapshot belongs. ++test('#P-hardlink: a hard link at the snapshot temp name is not written through', async (t) => { ++ const home = mkdtempSync(join(tmpdir(), 'ds-bot-memory-')) ++ const outside = mkdtempSync(join(tmpdir(), 'ds-bot-memory-outside-')) ++ const target = join(outside, 'snapshot.md') ++ writeFileSync(target, 'PRE-EXISTING LINE\n') ++ const snapshots = join(home, 'memory', 'snapshots') ++ mkdirSync(snapshots, { recursive: true }) ++ const snapshot = join(snapshots, 'b2.md') ++ const temp = `${snapshot}.tmp` ++ linkSync(target, temp) ++ t.after(() => rmSync(outside, { recursive: true, force: true })) ++ const lines = [] ++ const original = console.error ++ console.error = (...args) => { lines.push(args.join(' ')) } ++ t.after(() => { console.error = original }) ++ const { section, call, stop } = start({ home }) ++ t.after(() => stop()) ++ await call('state', {}) ++ const text = section('b2') ++ assert.match(text, /The team memory is shared by every Bot/, 'the section is built even when it cannot be frozen') ++ assert.equal(readFileSync(target, 'utf8'), 'PRE-EXISTING LINE\n', 'the shared inode is byte-identical') ++ assert.equal(lstatSync(target).nlink, 2, 'the outside file still carries the temp name') ++ assert.equal(existsSync(snapshot), false, 'the shared file was not renamed into the snapshot place') ++ const warned = lines.filter(line => line.includes('not a plain file')) ++ assert.equal(warned.length, 1, 'said once for the temp name') ++ assert.ok(warned[0].includes(temp), `the warning names the temp name: ${warned[0]}`) ++}) diff --git a/test/model-menu.test.js b/test/model-menu.test.js --- a/test/model-menu.test.js +++ b/test/model-menu.test.js diff --git a/patches/ds-bot/0002-flybot-api-host.patch b/patches/ds-bot/0002-flybot-api-host.patch index e3f1ae88d..f1488739b 100644 --- a/patches/ds-bot/0002-flybot-api-host.patch +++ b/patches/ds-bot/0002-flybot-api-host.patch @@ -1591,7 +1591,7 @@ diff --git a/src/host/store.js b/src/host/store.js const temp = `${statePath}.${process.pid}.tmp` + // The temp name is a leaf like the state file itself: a link there would take the team + // outside the home and the rename would put the link over state.json (temp-name sweep). -+ if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file, so the Bot team was not written there.`) ++ if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file of its own, so the Bot team was not written there.`) try { await mkdir(home, { recursive: true }) const ensureLock = () => { diff --git a/patches/ds-bot/0014-flybot-branding.patch b/patches/ds-bot/0014-flybot-branding.patch index d49ce1227..f1955dffc 100644 --- a/patches/ds-bot/0014-flybot-branding.patch +++ b/patches/ds-bot/0014-flybot-branding.patch @@ -2046,7 +2046,7 @@ diff --git a/test/store.test.js b/test/store.test.js rt.state.prefs.marker = 'not saved' await rt.save() assert.equal(fs.readFileSync(statePath, 'utf8'), original) -@@ -285,3 +285,73 @@ test('dispose waits for a pending save and prevents later writes', async (t) => +@@ -285,3 +285,94 @@ test('dispose waits for a pending save and prevents later writes', async (t) => restore() } }) @@ -2120,6 +2120,27 @@ diff --git a/test/store.test.js b/test/store.test.js + assert.equal(fs.lstatSync(statePath).isSymbolicLink(), false, 'the link is replaced by the saved file') + assert.equal(JSON.parse(fs.readFileSync(statePath, 'utf8')).prefs.sweep, 'kept') +}) ++ ++// The temp name can also be a hard link: `lstat` reports a plain file, so a link-only check lets it ++// through and the save lands in the inode the other name holds (`nlink 2`). A name that is a file ++// but not only this one is refused the same way; the leftover plain temp of a crashed host (one ++// name, `nlink 1`) stays replaceable, which the next test keeps. ++test('a hard link at the state temp name is refused; the save never reaches the shared file', async (t) => { ++ const { rt, home, statePath, warnings } = runtime(t) ++ await rt.load() ++ await rt.save() ++ const before = fs.readFileSync(statePath, 'utf8') ++ const outside = join(home, 'outside-state.json') ++ fs.writeFileSync(outside, 'OUTSIDE\n') ++ fs.linkSync(outside, `${statePath}.${process.pid}.tmp`) ++ rt.state.prefs.sweep = 'not saved' ++ await assert.rejects(rt.save()) ++ assert.equal(fs.readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the shared inode keeps its bytes') ++ assert.equal(fs.lstatSync(outside).nlink, 2, 'the outside file still carries the temp name') ++ assert.equal(fs.readFileSync(statePath, 'utf8'), before, 'the state file kept what it had') ++ assert.ok(warnings.some(line => /not a plain file/.test(line.join(' '))), 'the refusal is named') ++ assert.ok(warnings.some(line => line.join(' ').includes(`${statePath}.${process.pid}.tmp`)), 'the warning names the temp path') ++}) diff --git a/test/update.test.js b/test/update.test.js --- a/test/update.test.js +++ b/test/update.test.js diff --git a/patches/ds-bot/files/src/host/automation-default.js b/patches/ds-bot/files/src/host/automation-default.js index 1d441947c..a73c2d416 100644 --- a/patches/ds-bot/files/src/host/automation-default.js +++ b/patches/ds-bot/files/src/host/automation-default.js @@ -37,7 +37,7 @@ async function writeOffers(file, offers) { const temp = `${file}.${process.pid}.tmp` // The temp name is a leaf like the defaults file: a link there would take the offer outside // the team folder and the rename would put the link over dsh-defaults.json (temp-name sweep). - if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file, so nothing was written there.`) + if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file of its own, so nothing was written there.`) try { await writeFile(temp, `${JSON.stringify({ version: 1, offers }, null, 2)}\n`, 'utf8') await rename(temp, file) diff --git a/patches/ds-bot/files/src/host/decisions.js b/patches/ds-bot/files/src/host/decisions.js index fe805489a..9e0d6ea17 100644 --- a/patches/ds-bot/files/src/host/decisions.js +++ b/patches/ds-bot/files/src/host/decisions.js @@ -13,7 +13,7 @@ import { dirname, join } from 'node:path' import { cut } from './text.js' import { defineTool } from './tool-schema.js' import { looksSecret } from './memory.js' -import { fileKind } from './real-dir.js' +import { fileKind, tempWritable } from './real-dir.js' import { output } from './tools.js' const OPTION_MAX = 200 @@ -78,11 +78,11 @@ export function install(rt) { const next = kept.length > 0 ? `${kept.join('\n')}\n` : '' if (next !== text) { const tmp = `${path}.tmp` - if (await fileKind(tmp) === 'other') { + if (!tempWritable(tmp)) { trimRefused = true if (!refusedTemp) { refusedTemp = true - rt.warn('ds-bot: the decision log was not trimmed: %s is not a plain file', tmp) + rt.warn('ds-bot: the decision log was not trimmed: %s is not a plain file of its own', tmp) } return kept.length } @@ -162,7 +162,7 @@ export function install(rt) { if (wrote === 'refused') return 'Not recorded: the decision log is not a plain file.' // The feed payload drops the id to a safe shape and adds the live botId (a consumer keys by it). rt.fireFeed?.('decision', { botId, option, choice, ...(why ? { why } : {}), time: entry.time }) - if (wrote === 'untrimmed') return `Written: ${choice} "${option}"${why ? ` — ${why}` : ''} — the log was not trimmed: ${path}.tmp is not a plain file.` + if (wrote === 'untrimmed') return `Written: ${choice} "${option}"${why ? ` — ${why}` : ''} — the log was not trimmed: ${path}.tmp is not a plain file of its own.` return `Recorded: ${choice} "${option}"${why ? ` — ${why}` : ''}.` }, })) diff --git a/patches/ds-bot/files/src/host/real-dir.js b/patches/ds-bot/files/src/host/real-dir.js index de82dad09..60c4a123d 100644 --- a/patches/ds-bot/files/src/host/real-dir.js +++ b/patches/ds-bot/files/src/host/real-dir.js @@ -51,9 +51,19 @@ export async function fileKind(path) { // `writeFile` follows a link at the last component, so a link left at `..tmp` would take // the bytes outside the home and the `rename` after them would put the link where the file belongs. // `absent` is a writer's to create and a plain leftover under this host's own pid is replaceable; -// anything else refuses the write before the first byte. The destination itself needs no such -// check: `rename` replaces the name, link included, instead of writing through it. -export const tempWritable = (path) => fileKindSync(path) !== 'other' +// anything else refuses the write before the first byte. A hard link is a plain file to `lstat` +// while its bytes are also a name outside this home (a write lands in the shared inode, not in a +// link), so a name with another name (`nlink > 1`) is refused the same way; a crashed host's own +// leftover is one name and stays replaceable. The destination itself needs no such check: `rename` +// replaces the name, link included, instead of writing through it. +export const tempWritable = (path) => { + try { + const info = lstatSync(path) + return info.isFile() && !info.isSymbolicLink() && info.nlink === 1 + } catch (error) { + return error?.code === 'ENOENT' + } +} export function fileKindSync(path) { try { diff --git a/patches/ds-bot/files/test/automation-default.test.js b/patches/ds-bot/files/test/automation-default.test.js index 12c4cdeb4..54daa21f0 100644 --- a/patches/ds-bot/files/test/automation-default.test.js +++ b/patches/ds-bot/files/test/automation-default.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { existsSync, lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { existsSync, linkSync, lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' import { test } from 'node:test' @@ -161,3 +161,23 @@ test('a linked defaults temp name does not take the offer; the file is not creat assert.equal(existsSync(file) ? lstatSync(file).isSymbolicLink() : false, false, 'the defaults file is not a link') assert.ok(warnings.some(line => /not a plain file/.test(line)), warnings.join('\n')) }) + +// The temp name can be a hard link, which `lstat` reports as a plain file: the offer would land in +// the inode the other name holds. It is refused before the write, once, and the bundle is not +// switched on. +test('a hard link at the defaults temp name keeps the offer unwritten', async (t) => { + const file = scratch(t) + const outside = join(dirname(file), 'outside-defaults.json') + writeFileSync(outside, 'OUTSIDE\n') + linkSync(outside, `${file}.${process.pid}.tmp`) + const warnings = [] + const plugins = manager() + const outcome = await offerBundle({ manager: plugins, profile: 'web', file, warn: (...args) => warnings.push(args.join(' ')) }) + assert.equal(outcome, 'unrecorded') + assert.equal(plugins.on, false, 'the bundle is not switched on') + assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the shared inode keeps its bytes') + assert.equal(lstatSync(outside).nlink, 2, 'the outside file still carries the temp name') + assert.equal(existsSync(file), false, 'the defaults file is not created') + assert.ok(warnings.some(line => /not a plain file/.test(line)), warnings.join('\n')) + assert.ok(warnings.some(line => line.includes(`${file}.${process.pid}.tmp`)), 'the warning names the temp path') +}) diff --git a/patches/ds-bot/files/test/decisions.test.js b/patches/ds-bot/files/test/decisions.test.js index 6ae241478..09e53b4c9 100644 --- a/patches/ds-bot/files/test/decisions.test.js +++ b/patches/ds-bot/files/test/decisions.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { existsSync, lstatSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { existsSync, linkSync, lstatSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -180,3 +180,35 @@ test('#2103: a linked temp name does not take the trim; the log stays a plain fi assert.equal(existsSync(`${plain}.tmp`), false, 'the healthy trim renames its temp name away') assert.deepEqual(healthyWarnings, []) }) + +// The trim's temp name can be a hard link, which `lstat` reports as a plain file: the retained log +// would be written into the inode the other name holds. It is refused before the write, warned +// once, and the log keeps its lines. +test('#P-hardlink: a hard link at the trim temp name does not take the trim', async (t) => { + const line = (n) => `${JSON.stringify({ time: 1_000 + n, bot: 'a', botName: 'Chief', option: `kept ${n}`, choice: 'used' })}\n` + const full = (n) => Array.from({ length: n }, (_, i) => line(i)).join('') + + const { ctx, home, tool, stop } = await boot({ decisions: true }) + t.after(stop) + const elsewhere = mkdtempSync(join(tmpdir(), 'ds-bot-decisions-tmp-target-')) + t.after(() => rmSync(elsewhere, { recursive: true, force: true, maxRetries: 5 })) + const target = join(elsewhere, 'outside.txt') + const before = 'PRE-EXISTING BYTES\n' + writeFileSync(target, before) + const log = join(home, 'decisions.jsonl') + writeFileSync(log, full(2_000)) + linkSync(target, `${log}.tmp`) + + const warnings = [] + ctx.logger.warn = (...args) => warnings.push(args.join(' ')) + const first = await tool('record_decision', { option: 'Trigger the trim', choice: 'used' }) + + assert.equal(readFileSync(target, 'utf8'), before, 'the shared inode keeps its bytes') + assert.equal(lstatSync(target).nlink, 2, 'the outside file still carries the temp name') + const lines = readFileSync(log, 'utf8').split('\n').filter(Boolean) + assert.equal(lines.length, 2_001, 'the refused trim leaves the log as it was, plus the new line') + assert.doesNotMatch(first, /^Recorded:/, 'the answer does not claim a clean record') + assert.equal(warnings.length, 1, 'one warning for the temp name') + assert.match(warnings[0], /decisions\.jsonl\.tmp/) + assert.match(warnings[0], /not a plain file/) +}) diff --git a/patches/ds-bot/files/test/real-dir.test.js b/patches/ds-bot/files/test/real-dir.test.js index 59687d736..d924a00e0 100644 --- a/patches/ds-bot/files/test/real-dir.test.js +++ b/patches/ds-bot/files/test/real-dir.test.js @@ -3,11 +3,11 @@ // states: a real directory here, nothing there at all, or something else that must not be read // through. `#2056` / `#2057` call the same function instead of writing the check again. import assert from 'node:assert/strict' -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { linkSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' -import { dirKind, dirKindSync, fileKind, fileKindSync } from '../src/host/real-dir.js' +import { dirKind, dirKindSync, fileKind, fileKindSync, tempWritable } from '../src/host/real-dir.js' function bench() { const dir = mkdtempSync(join(tmpdir(), 'ds-bot-real-dir-')) @@ -71,3 +71,28 @@ test('#2079: a leaf file is real, absent, or other, and a link is never a file h assert.equal(fileKindSync(join(dir, 'a-folder')), 'other') } finally { done() } }) + +// The temp name a write lands through is asked in one place (temp-name sweep). A missing name is the +// writer's to create and the plain leftover of this host's own crash is replaceable, so a rejected +// write never leaves a name that locks the file forever; a link, a folder, or a file another name +// also holds (`nlink 2`, which `lstat` reports as a plain file) refuses before the first byte. +test('#P-hardlink: a temp name is writable only when it is absent or a plain file of its own', () => { + const { dir, outside, done } = bench() + try { + const temp = join(dir, 'state.json.123.tmp') + assert.equal(tempWritable(temp), true, 'absent is a writer\'s to create') + writeFileSync(join(dir, 'not-a-dir'), 'x') + assert.equal(tempWritable(join(dir, 'not-a-dir', 'state.json.123.tmp')), false, 'a check that cannot run (ENOTDIR) is not a licence') + writeFileSync(temp, 'left by a crash\n') + assert.equal(tempWritable(temp), true, 'a plain leftover under this host\'s own pid is replaceable') + const shared = join(outside, 'shared.md') + writeFileSync(shared, 'OUTSIDE\n') + linkSync(shared, join(dir, 'shared.json.123.tmp')) + assert.equal(tempWritable(join(dir, 'shared.json.123.tmp')), false, 'a hard link shares its bytes with a name outside this home') + assert.equal(tempWritable(shared), false, 'and the other name is the same inode, both report nlink 2') + symlinkSync(shared, join(dir, 'linked.json.123.tmp')) + assert.equal(tempWritable(join(dir, 'linked.json.123.tmp')), false, 'a link is not a file here') + mkdirSync(join(dir, 'folder.json.123.tmp')) + assert.equal(tempWritable(join(dir, 'folder.json.123.tmp')), false, 'a folder where a file belongs is refused too') + } finally { done() } +}) diff --git a/plugins/social/src/server-config.js b/plugins/social/src/server-config.js index da1312ae5..3dbc9b505 100644 --- a/plugins/social/src/server-config.js +++ b/plugins/social/src/server-config.js @@ -70,15 +70,19 @@ export async function probeServer(server, { fetch: fetchImpl = socialFetch, time // The temp name a private file lands through is a leaf like the file itself (temp-name sweep): a // link left at `..tmp` would take the contents outside the team folder and the rename // would put the link where the file belongs. Absent is ours to create, a plain leftover is -// replaceable, and anything else refuses the write before the first byte. +// replaceable, and anything else refuses the write before the first byte. A hard link is a plain +// file to `lstat` but shares its bytes with another name (`nlink > 1`), so it refuses too. const tempWritable = (path) => { - try { return lstatSync(path).isFile() } catch (error) { return error?.code === 'ENOENT' } + try { + const info = lstatSync(path) + return info.isFile() && !info.isSymbolicLink() && info.nlink === 1 + } catch (error) { return error?.code === 'ENOENT' } } const writePrivate = (path, value) => { mkdirSync(dirname(path), { recursive: true }) const temp = `${path}.${process.pid}.tmp` - if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file, so nothing was written there.`) + if (!tempWritable(temp)) throw new Error(`Not saved: ${temp} is not a plain file of its own, so nothing was written there.`) writeFileSync(temp, JSON.stringify(value, null, 2), { mode: 0o600 }) renameSync(temp, path) try { chmodSync(path, 0o600) } catch { /* file systems that ignore the write mode */ } diff --git a/plugins/social/test/server-config-temp.test.js b/plugins/social/test/server-config-temp.test.js index e0d16d864..f1a20a2bf 100644 --- a/plugins/social/test/server-config-temp.test.js +++ b/plugins/social/test/server-config-temp.test.js @@ -3,7 +3,7 @@ // (temp-name sweep): a link left there would take the choice outside the team folder and the // rename would put the link where the override belongs. import assert from 'node:assert/strict' -import { existsSync, lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { existsSync, linkSync, lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -38,3 +38,29 @@ test('a linked server-override temp name does not take the choice; the file stay rmSync(home, { recursive: true, force: true }) } }) + +// The same temp name as a hard link is a plain file to `lstat`, so the choice would land in the +// inode the other name holds. The refusal names the temp path and the override stays absent. +test('a hard link at the server-override temp name does not take the choice', async () => { + const home = mkdtempSync(join(tmpdir(), 'flybot-social-temp-')) + const controller = createServerController({ + base: { server: 'https://a.test', credentialsPath: join(home, 'flybot-social.json'), displayName: 'Me' }, + flybot: fly(), + probe: async server => ({ name: 'Bot Social', features: [], server }), + start: async () => ({ home: { handle: '@me.home' }, dispose: () => {} }), + startTimeoutMs: 20, + }) + try { + const outside = join(home, 'outside-server.json') + writeFileSync(outside, 'OUTSIDE\n') + linkSync(outside, join(home, `flybot-social-server.json.${process.pid}.tmp`)) + await assert.rejects(controller.set({ server: 'https://b.test', confirm: true }), /not a plain file/) + assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the shared inode keeps its bytes') + assert.equal(lstatSync(outside).nlink, 2, 'the outside file still carries the temp name') + const override = join(home, 'flybot-social-server.json') + assert.equal(existsSync(override), false, 'the override is not created') + } finally { + controller.dispose() + rmSync(home, { recursive: true, force: true }) + } +}) diff --git a/plugins/workflow/src/service.js b/plugins/workflow/src/service.js index 913f5cc0b..30554fd3c 100644 --- a/plugins/workflow/src/service.js +++ b/plugins/workflow/src/service.js @@ -192,7 +192,10 @@ function blankRun(workflow, task, now, history = []) { // the link where the store belongs. Absent is ours to create, a plain leftover is replaceable, and // anything else refuses the write before the first byte. const tempWritable = (path) => { - try { return lstatSync(path).isFile() } catch (error) { return error?.code === 'ENOENT' } + try { + const info = lstatSync(path) + return info.isFile() && !info.isSymbolicLink() && info.nlink === 1 + } catch (error) { return error?.code === 'ENOENT' } } export function createWorkflowService({ @@ -319,7 +322,7 @@ export function createWorkflowService({ state.seq = seq mkdirSync(dir, { recursive: true }) const tmp = `${file}.${process.pid}.tmp` - if (!tempWritable(tmp)) throw new Error(`Not saved: ${tmp} is not a plain file, so the workflow store was not written there.`) + if (!tempWritable(tmp)) throw new Error(`Not saved: ${tmp} is not a plain file of its own, so the workflow store was not written there.`) writeFileSync(tmp, JSON.stringify(state)) renameSync(tmp, file) } diff --git a/plugins/workflow/test/temp-name.test.js b/plugins/workflow/test/temp-name.test.js index 713a0a1c4..712d873e5 100644 --- a/plugins/workflow/test/temp-name.test.js +++ b/plugins/workflow/test/temp-name.test.js @@ -2,7 +2,7 @@ // That temp name is a leaf like the store (temp-name sweep): a link left there would take the // workflows outside their folder and the rename would put the link where the store belongs. import assert from 'node:assert/strict' -import { existsSync, lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { existsSync, linkSync, lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -26,3 +26,24 @@ test('a linked workflow-store temp name does not take the store; workflows.json rmSync(dir, { recursive: true, force: true }) } }) + +// A hard link at the temp name is a plain file to `lstat`, so the store would land in the shared +// inode. It is refused, and workflows.json is not created. +test('a hard link at the workflow-store temp name does not take the store', () => { + const dir = mkdtempSync(join(tmpdir(), 'flybot-wf-temp-')) + try { + const svc = createWorkflowService({ dir, persist: true, now: () => 5, bots: () => [] }) + const outside = join(dir, 'outside-workflows.json') + writeFileSync(outside, 'OUTSIDE\n') + linkSync(outside, join(dir, `workflows.json.${process.pid}.tmp`)) + const saved = svc.setConcurrency({ workflowId: DELIVERY_ID, maxConcurrent: 3 }) + assert.equal(saved.ok, false, 'the write is refused') + assert.match(saved.error, /not a plain file/) + assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the shared inode keeps its bytes') + assert.equal(lstatSync(outside).nlink, 2, 'the outside file still carries the temp name') + const file = join(dir, 'workflows.json') + assert.equal(existsSync(file), false, 'the store is not created') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +})