fix(host): ask the temp name before five writes land through it

changelog.d/2109-temp-name-sweep.fixed.md

@@ -0,0 +1 @@
+A link left at the temporary name a write lands through no longer takes the write: the team state, the OCR cache, the Automation-tasks defaults, the Bot Social server choice and the workflow store ask `<file>.<pid>.tmp` as a file before writing, so a link (or a folder) there refuses the save, with the temp name in the message, instead of putting the bytes outside the folder and renaming the link into place. A window between the check and the write remains, as with every `lstat`-then-write guard. ([#2109](https://github.com/ThinkFlowLab/FlyBot/issues/2109)).

core/src/host/automation-default.js

@@ -8,6 +8,7 @@
 // A DSH that does not offer the bundle is left alone, and so is a second dsh on the same data.
 import { mkdir, readFile, rename, unlink, writeFile } from 'node:fs/promises'
 import { join } from 'node:path'
+import { tempWritable } from './real-dir.js'
 
 export const SCHEDULE_BUNDLE = '@deepseek-ai/dsh-experimental-schedule-bundle'
 const MARKER = 'dsh-defaults.json'
@@ -34,6 +35,9 @@ export async function readOffers(file) {
 async function writeOffers(file, offers) {
   await mkdir(join(file, '..'), { recursive: true })
   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.`)
   try {
     await writeFile(temp, `${JSON.stringify({ version: 1, offers }, null, 2)}\n`, 'utf8')
     await rename(temp, file)
@@ -50,7 +54,7 @@ async function writeOffers(file, offers) {
 //   'unrecorded'  the offer could not be written down, so nothing was changed
 //   'enabled'     switched on
 //   'failed'      the manager refused; the note is withdrawn so the next start tries again
-export async function offerBundle({ manager, bundle = SCHEDULE_BUNDLE, profile, file, now = () => new Date() }) {
+export async function offerBundle({ manager, bundle = SCHEDULE_BUNDLE, profile, file, now = () => new Date(), warn = () => {} }) {
   let offers
   try {
     offers = await readOffers(file)
@@ -70,7 +74,8 @@ export async function offerBundle({ manager, bundle = SCHEDULE_BUNDLE, profile,
   }
   try {
     await note(now().toISOString())
-  } catch {
+  } catch (error) {
+    warn('ds-bot: the Automation tasks offer was not written down: %s', error?.message ?? error)
     return 'unrecorded'
   }
   if (entry.enabled === true) return 'already-on'
@@ -91,7 +96,7 @@ export function install(rt) {
     // The window that shows the team without owning it leaves the profile to the other one.
     if (rt.readOnly() !== undefined) return
     const profile = ctx.get?.('profileContext')?.name ?? 'default'
-    const outcome = await offerBundle({ manager: ctx.get?.('pluginManager'), profile, file: join(home, MARKER) })
+    const outcome = await offerBundle({ manager: ctx.get?.('pluginManager'), profile, file: join(home, MARKER), warn: rt.warn })
     if (outcome === 'failed') rt.warn('ds-bot: could not switch on Automation tasks; switch them on from the Plugins page')
   }
 

core/src/host/images.js

@@ -6,6 +6,7 @@ import { mkdir, readFile, rename, stat, writeFile } from 'node:fs/promises'
 import { join } from 'node:path'
 import { BlockAssembler } from '@deepseek-ai/dsh-llm'
 import { OCR_CACHE_MAX, OCR_PROMPT, OCR_RETRY_MS, OCR_TIMEOUT_MS } from './constants.js'
+import { tempWritable } from './real-dir.js'
 import { splitRef } from './text.js'
 
 export function offloadTargets(session, beforeSeq) {
@@ -131,6 +132,9 @@ export function install(rt) {
     ocrWriting = ocrWriting.then(async () => {
       await mkdir(home, { recursive: true })
       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.`)
       await writeFile(temp, snapshot, 'utf8')
       await rename(temp, ocrPath)
     }).catch(error => rt.warn('ds-bot: OCR cache write failed: %s', error))

core/src/host/real-dir.js

@@ -47,6 +47,14 @@ export async function fileKind(path) {
   }
 }
 
+// A temp name a write lands through is a leaf like the file it becomes (temp-name sweep): the
+// `writeFile` follows a link at the last component, so a link left at `<file>.<pid>.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'
+
 export function fileKindSync(path) {
   try {
     return fileKindOf(lstatSync(path))

core/src/host/store.js

@@ -8,6 +8,7 @@ import { join } from 'node:path'
 import { LEGACY_THEMES, MAX_ENTRY, MAX_EXCHANGES, ROOM_MODES } from './constants.js'
 import { homeLock } from './home-lock.js'
 import { openJournal } from './journal.js'
+import { tempWritable } from './real-dir.js'
 import { codePoints, currentColor, normalize, unquote } from './text.js'
 
 // The saved team's format version. Version 2 (#47) adds the identity schema: member
@@ -925,6 +926,9 @@ export function install(rt) {
     const snapshot = JSON.stringify(state, null, 2)
     const run = writing.then(async () => {
       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.`)
       try {
         await mkdir(home, { recursive: true })
         const ensureLock = () => {

core/test/automation-default.test.js

@@ -1,5 +1,5 @@
 import assert from 'node:assert/strict'
-import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
+import { existsSync, 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'
@@ -142,3 +142,22 @@ test('config.automationTasks = false never offers it', (t) => {
   assert.equal(ctx.calls.includes('effect ds-bot: automation tasks default'), false)
   assert.equal(rt.offerAutomationTasks, undefined)
 })
+
+// The defaults file lands through `<file>.<pid>.tmp` and is renamed into place. The temp name is a
+// leaf like the file (temp-name sweep): a link left there would take the offer outside the team
+// folder and the rename would put the link where `dsh-defaults.json` belongs. A refused write is
+// reported once, and the offer is not recorded.
+test('a linked defaults temp name does not take the offer; the file is not created', async (t) => {
+  const file = scratch(t)
+  const outside = join(dirname(file), 'outside-defaults.json')
+  writeFileSync(outside, 'OUTSIDE\n')
+  symlinkSync(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 when the offer cannot be written down')
+  assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the file behind the temp link is not written through')
+  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'))
+})

core/test/images.test.js

@@ -1,9 +1,10 @@
 import assert from 'node:assert/strict'
-import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
+import { existsSync, 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'
-import { attachmentFilePath, attachmentObjectPath, imagePlaceholder, offloadTargets } from '../src/host/images.js'
+import { attachmentFilePath, attachmentObjectPath, imagePlaceholder, install, offloadTargets } from '../src/host/images.js'
+import { fakeContext } from './fake-ctx.js'
 
 test('offloadTargets selects live images from prior user and tool messages', () => {
   const events = [
@@ -115,3 +116,72 @@ test('#779: the placeholder names the file, its size and its type; without a pat
   // An unnamed image keeps the sha prefix the host has always shown.
   assert.match(imagePlaceholder({ attachmentId: id }), /\[Image sha256:bdc77c70:/)
 })
+
+// The OCR cache lands through `<home>/ocr.json.<pid>.tmp` and is renamed into place. That temp name
+// is a leaf like the cache (temp-name sweep): `writeFile` follows a link there, so a pre-set link
+// would take the cache outside the home and the rename would put the link where `ocr.json` belongs.
+const OCR_ATTACHMENT = { attachmentId: `sha256:${'b'.repeat(64)}`, name: 'triangle.png' }
+
+function ocrRuntime(t) {
+  const home = mkdtempSync(join(tmpdir(), 'ds-bot-images-ocr-'))
+  const warnings = []
+  const ctx = fakeContext()
+  Object.assign(ctx, {
+    llm: {
+      listProviders: () => [{ id: 'deepseek', name: 'DeepSeek' }],
+      listModels: async () => [{ id: 'vision', input: ['text', 'image'] }],
+      resolveModelInfo: async () => ({ inputModalities: ['text', 'image'] }),
+      stream: async function* () {
+        yield { type: 'text-delta', index: 0, text: 'A red triangle.' }
+        yield { type: 'finish', reason: { kind: 'stop' } }
+      },
+    },
+  })
+  const rt = {
+    ctx,
+    home,
+    config: {},
+    warn: (...args) => warnings.push(args.join(' ')),
+    load: async () => {},
+    readOnly: () => undefined,
+    roomOf: () => 'r1',
+    isBotSession: () => true,
+    ownRequest: () => false,
+    catalog: async () => [{ ref: 'deepseek/vision', name: 'Vision', input: ['image'] }],
+    defaultRef: () => 'deepseek/vision',
+    tagUsage: messages => messages,
+  }
+  install(rt)
+  t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 }))
+  return { rt, home, warnings }
+}
+
+// The cache write is queued after the reading, so wait for the effect instead of guessing a delay.
+async function waitFor(check) {
+  for (let i = 0; i < 300; i += 1) {
+    if (await check()) return
+    await new Promise(resolve => setTimeout(resolve, 10))
+  }
+}
+
+test('a linked OCR cache temp name does not take the cache; ocr.json stays a plain file', async (t) => {
+  const { rt, home, warnings } = ocrRuntime(t)
+  const outside = join(home, 'outside-ocr.txt')
+  writeFileSync(outside, 'OUTSIDE\n')
+  symlinkSync(outside, join(home, `ocr.json.${process.pid}.tmp`))
+  await rt.imagesForGroup([OCR_ATTACHMENT], 'r1')
+  await waitFor(() => readFileSync(outside, 'utf8') !== 'OUTSIDE\n' || warnings.length > 0)
+  assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the file behind the temp link is not written through')
+  const cache = join(home, 'ocr.json')
+  assert.equal(existsSync(cache) ? lstatSync(cache).isSymbolicLink() : false, false, 'the cache is not a link')
+  assert.ok(warnings.some(line => /not a plain file/.test(line)), warnings.join('\n'))
+})
+
+test('an ordinary OCR cache temp name is written and renamed as before', async (t) => {
+  const { rt, home, warnings } = ocrRuntime(t)
+  await rt.imagesForGroup([OCR_ATTACHMENT], 'r1')
+  const cache = join(home, 'ocr.json')
+  await waitFor(() => existsSync(cache))
+  assert.equal(warnings.length, 0)
+  assert.equal(JSON.parse(readFileSync(cache, 'utf8'))[OCR_ATTACHMENT.attachmentId].text, 'A red triangle.')
+})

core/test/roster.test.js

@@ -541,7 +541,10 @@ test('a write that fails at save time archives the Session it made', async (t) =
   ctx.workspaceRegistry.archiveSession = async (id) => { archived.push(id); return archive(id) }
   const answer = await call('create-bot', { name: 'Phantom' })
   assert.equal(answer.ok, false)
-  assert.equal(answer.error.code, 'bot/internal')
+  // The temp-name guard refuses before `writeFile` reaches EISDIR, so the failure is a refusal
+  // that names the temp path; the rollback this test is about is unchanged.
+  assert.equal(answer.error.code, 'bot/refused')
+  assert.match(answer.error.message, /not a plain file/)
   assert.deepEqual(archived, created)
   const team = (await call('state', {})).value
   assert.equal(team.bots.some(bot => bot.name === 'Phantom'), false)

core/test/store.test.js

@@ -68,7 +68,7 @@ test('a failed state write rejects and leaves the saved file unchanged', async (
   const original = fs.readFileSync(statePath)
   fs.mkdirSync(`${statePath}.${process.pid}.tmp`)
   rt.state.prefs.marker = 'not saved'
-  await assert.rejects(rt.save(), { code: 'EISDIR' })
+  await assert.rejects(rt.save())
   assert.deepEqual(fs.readFileSync(statePath), original)
 })
 
@@ -320,3 +320,38 @@ test('a roster id that is not an own key finds no Bot and no group (#1704)', asy
   assert.equal(rt.findBot('a').name, 'Chief')
   assert.equal(rt.findRooms('Ops')[0].id, 'r1')
 })
+
+// The state write lands through `<state>.json.<pid>.tmp` and then renames it into place. The temp
+// name is a leaf like the state file (temp-name sweep): `writeFile` would follow a link there, put
+// the team outside the home and then rename the link over the state file. Only the target is
+// replaced by `rename`, link included, never written through.
+test('a linked state temp name does not take the save; the state file stays a plain file', async (t) => {
+  const { rt, home, statePath, warnings } = runtime(t)
+  await rt.load()
+  const outside = join(home, 'outside-state.json')
+  fs.writeFileSync(outside, 'OUTSIDE\n')
+  fs.symlinkSync(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 file behind the temp link is not written through')
+  assert.equal(fs.lstatSync(statePath).isSymbolicLink(), false, 'the state file is still a plain file')
+  assert.equal(JSON.parse(fs.readFileSync(statePath, 'utf8')).prefs.sweep, undefined, 'the refused save did not land')
+  assert.ok(warnings.some(line => /not a plain file/.test(line.join(' '))), 'the refusal is named')
+})
+
+// The other half of the same shape: a link at the destination itself is replaced by the rename, so
+// it never needs the temp check. This is why the sweep only adds the temp name.
+test('a link at the state file itself is replaced, not written through', async (t) => {
+  const { rt, home, statePath } = runtime(t)
+  await rt.load()
+  await rt.save()
+  const outside = join(home, 'outside-state.json')
+  fs.writeFileSync(outside, 'OUTSIDE\n')
+  fs.rmSync(statePath)
+  fs.symlinkSync(outside, statePath)
+  rt.state.prefs.sweep = 'kept'
+  await rt.save()
+  assert.equal(fs.readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the rename never follows the target link')
+  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')
+})