Decisions and diagnostics logs are written through when the leaf is a link

changelog.d/2089-decisions-diagnostics-leaf.fixed.md

@@ -0,0 +1 @@
+The decision log and the diagnostics log now ask what their file is before they write, the way the state journal does: a `decisions.jsonl` or `logs/diagnostics.jsonl` swapped for a link is refused instead of appending the line into whatever the link points at. A refused decision is dropped with one warning and the tool answers `Not recorded: the decision log is not a plain file.`; the diagnostics log drops the line with one line on stderr, keeps it readable in memory, and `clear` no longer truncates a file behind such a link.

core/src/host/decisions.js

@@ -13,6 +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 { output } from './tools.js'
 
 const OPTION_MAX = 200
@@ -39,6 +40,18 @@ export function install(rt) {
   // Lines on disk after the last append/trim, so a record does not re-read the file every time
   // (null until the first record counts once).
   let linesOnDisk = null
+  // The log is one leaf file, and a link there is not a place to write (#2089). The check is inside
+  // the chain, so a queued record cannot slip past it, and one refused file is said once rather
+  // than once per decision.
+  let refusedLeaf = false
+  const leafWritable = async () => {
+    if (await fileKind(path) !== 'other') return true
+    if (!refusedLeaf) {
+      refusedLeaf = true
+      rt.warn('ds-bot: the decision log is not a plain file, so a decision was not written to it')
+    }
+    return false
+  }
   const countLines = async () => (await readFile(path, 'utf8')).split('\n').filter(Boolean).length
   // Keep the file bounded in place: the newest entries under both caps, written through a temp
   // file so a reader never sees a half-trimmed log. The Bot side stays append-only; this is the
@@ -67,12 +80,14 @@ export function install(rt) {
   function record(entry) {
     const line = `${JSON.stringify(entry)}\n`
     writing = writing.then(async () => {
+      if (!await leafWritable()) return 'refused'
       await mkdir(dirname(path), { recursive: true })
       await appendFile(path, line, 'utf8')
       linesOnDisk = linesOnDisk === null ? await countLines() : linesOnDisk + 1
       const { size } = await stat(path)
       if (linesOnDisk > KEPT || size > MAX_BYTES) linesOnDisk = await trim()
-    }).catch(error => rt.warn('ds-bot: a decision was not written: %s', error))
+      return 'written'
+    }).catch(error => { rt.warn('ds-bot: a decision was not written: %s', error); return 'failed' })
     return writing
   }
   // The entries, oldest first, capped to the newest `KEPT` lines and `MAX_BYTES`. A read takes at
@@ -124,7 +139,10 @@ export function install(rt) {
       if (looksSecret(option) || looksSecret(why)) return 'A decision never holds a key, token or password; record the choice without it.'
       const safeName = NAME_SAFE.test(botId) ? botId : 'bot'
       const entry = { time: Date.now(), bot: safeName, botName: oneLine(bot.name, 40), option, choice, ...(why ? { why } : {}) }
-      await record(entry)
+      const wrote = await record(entry)
+      // A refused leaf drops the line, so the answer and the feed say the same thing: nothing was
+      // recorded. A write that failed for another reason keeps its old shape (a warning, an answer).
+      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 })
       return `Recorded: ${choice} "${option}"${why ? ` — ${why}` : ''}.`

core/src/host/diagnostics.js

@@ -10,6 +10,7 @@ import { arch, homedir, platform, release } from 'node:os'
 import { dirname, join } from 'node:path'
 import { fileURLToPath } from 'node:url'
 import { format } from 'node:util'
+import { fileKind } from './real-dir.js'
 import { cut } from './text.js'
 
 export const KEEP = 200
@@ -136,9 +137,20 @@ export function install(rt) {
     : Promise.resolve([])
   let cleared = 0
 
+  // The log is one leaf file under the home, so a link there is not a place to write (#2089). The
+  // check sits inside the chain, before the append and before the size rename, so a linked leaf is
+  // left alone entirely; one line on stderr per host says so, without the home folder in it.
+  let refusedLeaf = false
+  const refuseLeaf = () => {
+    if (refusedLeaf) return
+    refusedLeaf = true
+    console.error('[ds-bot] diagnostics log is not a plain file; nothing was written there')
+  }
+
   const append = (entry) => {
     if (!toFile) return
     writing = writing.then(async () => {
+      if (await fileKind(logPath) === 'other') { refuseLeaf(); return }
       await mkdir(dirname(logPath), { recursive: true })
       const size = await stat(logPath).then(info => info.size, () => 0)
       if (size > LOG_MAX_BYTES) await rename(logPath, `${logPath}.1`)
@@ -258,7 +270,12 @@ export function install(rt) {
     cleared = Date.now()
     fresh = []
     if (!toFile) return
-    writing = writing.then(() => writeFile(logPath, '', 'utf8')).catch(() => {})
+    // Clearing truncates the leaf, so it is the same question as the append: a link is not ours to
+    // truncate, and the old file behind it stays exactly as it was.
+    writing = writing.then(async () => {
+      if (await fileKind(logPath) === 'other') { refuseLeaf(); return }
+      await writeFile(logPath, '', 'utf8')
+    }).catch(() => {})
     await writing
   }
 

core/test/decisions.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, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
 import { test } from 'node:test'
@@ -97,3 +97,36 @@ test('#1912: an option cut at an emoji boundary stays well formed in the log', a
   assert.equal(entry.option.length <= 200, true, `the stored option must stay within 200 units, got ${entry.option.length}`)
   assert.equal(option.startsWith(entry.option), true, 'the stored option must be a prefix of the recorded one')
 })
+
+// The log is one leaf file beside the home, so a link there is not a place to write: the rule the
+// journal keeps (#2074) and the memory side record keeps (#2079). A refusal drops the line, warns
+// once, and tells the Bot it was not recorded rather than claiming a line that is not on disk.
+test('a linked decision log is refused, not written through', async (t) => {
+  const { ctx, home, tool, events, stop } = await boot({ decisions: true })
+  t.after(stop)
+  const elsewhere = mkdtempSync(join(tmpdir(), 'ds-bot-decisions-target-'))
+  t.after(() => rmSync(elsewhere, { recursive: true, force: true, maxRetries: 5 }))
+  const target = join(elsewhere, 'decisions.jsonl')
+  const before = 'PRE-EXISTING LINE\n'
+  writeFileSync(target, before)
+  const leaf = join(home, 'decisions.jsonl')
+  symlinkSync(target, leaf)
+
+  const warnings = []
+  ctx.logger.warn = (...args) => warnings.push(args.join(' '))
+  const first = await tool('record_decision', { option: 'Keep the link', choice: 'rejected' })
+  const second = await tool('record_decision', { option: 'Keep it again', choice: 'used' })
+  assert.equal(readFileSync(target, 'utf8'), before, 'the file behind the link is not written through')
+  assert.equal(lstatSync(leaf).isSymbolicLink(), true, 'the link is still the leaf')
+  assert.equal(readlinkSync(leaf), target)
+  assert.match(first, /Not recorded/, 'the Bot is told the decision is not on disk')
+  assert.match(second, /Not recorded/)
+  assert.equal(warnings.length, 1, 'one warning, not one for every refused record')
+  assert.match(warnings[0], /not a plain file/)
+  assert.deepEqual(events, [], 'a decision that is not on disk is not announced on the feed')
+
+  // A real leaf afterwards: the refusal is about this file, not a latch that stops the log.
+  rmSync(leaf)
+  assert.match(await tool('record_decision', { option: 'Write it now', choice: 'used' }), /^Recorded:/)
+  assert.equal(readFileSync(leaf, 'utf8').trim().split('\n').length, 1)
+})

core/test/diagnostics.test.js

@@ -1,5 +1,5 @@
 import assert from 'node:assert/strict'
-import { mkdtempSync, readFileSync, rmSync } from 'node:fs'
+import { lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'
 import fsp from 'node:fs/promises'
 import { syncBuiltinESMExports } from 'node:module'
 import { homedir, tmpdir } from 'node:os'
@@ -116,6 +116,43 @@ test('dispose waits for the queued log appends (#1983)', async (t) => {
   assert.equal(lines.length, 500, 'every warning queued before dispose is on disk when it resolves')
 })
 
+// The log is one leaf file under the home, so a link there is not a place to write — the rule the
+// journal keeps (#2074) and the memory side record keeps (#2079). A refusal drops the line and says
+// so once; the entries stay readable in memory, and `clear` must not truncate what is behind the link.
+test('a linked diagnostics log is refused, not written through', async (t) => {
+  const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-'))
+  t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 }))
+  const elsewhere = mkdtempSync(join(tmpdir(), 'ds-bot-diag-target-'))
+  t.after(() => rmSync(elsewhere, { recursive: true, force: true, maxRetries: 5 }))
+  mkdirSync(join(home, 'logs'), { recursive: true })
+  const target = join(elsewhere, 'diagnostics.jsonl')
+  const before = 'PRE-EXISTING LINE\n'
+  writeFileSync(target, before)
+  const leaf = join(home, 'logs', 'diagnostics.jsonl')
+  symlinkSync(target, leaf)
+
+  const stderr = []
+  const quiet = t.mock.method(console, 'error', (...args) => stderr.push(args.join(' ')))
+  const rt = host(home)
+  rt.warn('ds-bot: first entry')
+  rt.warn('ds-bot: second entry')
+  await rt.dispose()
+  const afterWrites = readFileSync(target, 'utf8')
+  const entries = await rt.diagnostics.entries()
+  await rt.diagnostics.clear()
+  const afterClear = readFileSync(target, 'utf8')
+  quiet.mock.restore()
+
+  assert.equal(afterWrites, before, 'the file behind the link is not written through')
+  assert.equal(afterClear, before, 'clear does not truncate what is behind the link')
+  assert.equal(lstatSync(leaf).isSymbolicLink(), true, 'the link is still the leaf')
+  assert.equal(readlinkSync(leaf), target)
+  assert.deepEqual(entries.map(entry => entry.text), ['first entry', 'second entry'], 'the entries are still readable in memory')
+  const refusals = stderr.filter(line => /not a plain file/.test(line))
+  assert.equal(refusals.length, 1, 'one refusal, not one for every dropped line')
+  assert.equal(refusals[0].includes(home), false, 'the refusal does not print the home folder')
+})
+
 // #1916: the browser-log lines in the report are bounded, and those bounds are code-unit ones:
 // a line cut inside an emoji is stored and shown as U+FFFD in a report meant for a public issue.
 test('a browser-log line cut at its bound never splits a surrogate pair (#1916)', async (t) => {

docs/subsystems/core-host.md

@@ -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 `<home>/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.
+- `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 `<home>/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.
 - **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
@@ -405,6 +405,7 @@ Work a member hands a Bot with `message_bot` from its group Session runs in a hi
 ### Diagnostics
 
 - Host diagnostics never depend on `ctx.logger`: DSH may export no logger, and a test or plugin-local host may carry one without `warn`. `rt.warn` still writes stderr and the diagnostics log in that case (`src/host/diagnostics.js`). Plugins reach the same log through `flybot.warn` (#68); see [plugin-runtime](plugin-runtime.md).
+- The log itself is one leaf file under the home, and a write asks it before it appends ([#2089](https://github.com/ThinkFlowLab/FlyBot/issues/2089)): when `logs/diagnostics.jsonl` is a link — or anything other than a plain file — the line is dropped with one line on stderr that carries no home folder, the entries stay readable in memory, and `clear` leaves a file behind such a link untouched rather than truncating it.
 
 ### Untrusted fences in every shared room
 

patches/ds-bot/0001-core-fixes.patch

@@ -4642,10 +4642,11 @@ diff --git a/src/host/connectors.js b/src/host/connectors.js
 diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js
 --- a/src/host/diagnostics.js
 +++ b/src/host/diagnostics.js
-@@ -10,13 +10,14 @@ import { arch, homedir, platform, release } from 'node:os'
+@@ -10,13 +10,15 @@ import { arch, homedir, platform, release } from 'node:os'
  import { dirname, join } from 'node:path'
  import { fileURLToPath } from 'node:url'
  import { format } from 'node:util'
++import { fileKind } from './real-dir.js'
 +import { cut } from './text.js'
 
  export const KEEP = 200
@@ -4698,6 +4699,27 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js
    return undefined
  }
 
+@@ -134,9 +134,20 @@ export function install(rt) {
+     : Promise.resolve([])
+   let cleared = 0
+
++  // The log is one leaf file under the home, so a link there is not a place to write (#2089). The
++  // check sits inside the chain, before the append and before the size rename, so a linked leaf is
++  // left alone entirely; one line on stderr per host says so, without the home folder in it.
++  let refusedLeaf = false
++  const refuseLeaf = () => {
++    if (refusedLeaf) return
++    refusedLeaf = true
++    console.error('[ds-bot] diagnostics log is not a plain file; nothing was written there')
++  }
++
+   const append = (entry) => {
+     if (!toFile) return
+     writing = writing.then(async () => {
++      if (await fileKind(logPath) === 'other') { refuseLeaf(); return }
+       await mkdir(dirname(logPath), { recursive: true })
+       const size = await stat(logPath).then(info => info.size, () => 0)
+       if (size > LOG_MAX_BYTES) await rename(logPath, `${logPath}.1`)
 @@ -148,6 +148,19 @@ export function install(rt) {
      })
    }
@@ -4761,6 +4783,20 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js
      const list = await entries()
      return list.map(entry => (entry.stack ? `${entryLine(entry)}\n${entry.stack.replace(/^/gm, '    ')}` : entryLine(entry))).join('\n')
    }
+@@ -241,7 +241,12 @@ export function install(rt) {
+     cleared = Date.now()
+     fresh = []
+     if (!toFile) return
+-    writing = writing.then(() => writeFile(logPath, '', 'utf8')).catch(() => {})
++    // Clearing truncates the leaf, so it is the same question as the append: a link is not ours to
++    // truncate, and the old file behind it stays exactly as it was.
++    writing = writing.then(async () => {
++      if (await fileKind(logPath) === 'other') { refuseLeaf(); return }
++      await writeFile(logPath, '', 'utf8')
++    }).catch(() => {})
+     await writing
+   }
+
 diff --git a/src/host/images.js b/src/host/images.js
 --- a/src/host/images.js
 +++ b/src/host/images.js