fix(host): ask the decision log's temp name before the trim lands through it (#2103)

changelog.d/2103-decisions-trim-temp.fixed.md

@@ -0,0 +1 @@
+The decision log's housekeeping no longer writes through a link left at its temporary name: it lands through `<home>/decisions.jsonl.tmp` and renames it over the log, and that name is now asked as a file before the write. A link there refuses the trim — one warning, the new entry is still appended and the retained log simply grows past its bound — instead of writing the whole retained log outside the home and replacing the log itself with the link; the tool then answers `Written: … — the log was not trimmed` rather than `Recorded:` ([#2103](https://github.com/ThinkFlowLab/FlyBot/issues/2103)). A window between the check and the write remains, as with every `lstat`-then-write guard.

core/src/host/decisions.js

@@ -44,6 +44,11 @@ export function install(rt) {
   // 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
+  // Housekeeping writes a temp name and renames it over the log, so that name is a write target too
+  // (#2103): a link left there would take the retained log and then become the log. One refusal is
+  // said once, the same way the leaf is.
+  let refusedTemp = false
+  let trimRefused = false
   const leafWritable = async () => {
     if (await fileKind(path) !== 'other') return true
     if (!refusedLeaf) {
@@ -57,6 +62,7 @@ export function install(rt) {
   // file so a reader never sees a half-trimmed log. The Bot side stays append-only; this is the
   // host's own housekeeping.
   async function trim() {
+    trimRefused = false
     let text
     try { text = await readFile(path, 'utf8') } catch (error) { if (error?.code === 'ENOENT') return 0; throw error }
     const lines = text.split('\n').filter(Boolean)
@@ -72,6 +78,14 @@ 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') {
+        trimRefused = true
+        if (!refusedTemp) {
+          refusedTemp = true
+          rt.warn('ds-bot: the decision log was not trimmed: %s is not a plain file', tmp)
+        }
+        return kept.length
+      }
       await writeFile(tmp, next, 'utf8')
       await rename(tmp, path)
     }
@@ -85,7 +99,10 @@ export function install(rt) {
       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()
+      if (linesOnDisk > KEPT || size > MAX_BYTES) {
+        linesOnDisk = await trim()
+        if (trimRefused) return 'untrimmed'
+      }
       return 'written'
     }).catch(error => { rt.warn('ds-bot: a decision was not written: %s', error); return 'failed' })
     return writing
@@ -145,6 +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.`
       return `Recorded: ${choice} "${option}"${why ? ` — ${why}` : ''}.`
     },
   }))

core/test/decisions.test.js

@@ -130,3 +130,53 @@ test('a linked decision log is refused, not written through', async (t) => {
   assert.match(await tool('record_decision', { option: 'Write it now', choice: 'used' }), /^Recorded:/)
   assert.equal(readFileSync(leaf, 'utf8').trim().split('\n').length, 1)
 })
+
+// Housekeeping writes a temp name beside the log and renames it over the log, so the temp name is a
+// place this code writes (#2103). A link left there must be refused before the write, not taken:
+// otherwise the retained log is written through it and the log itself becomes a link.
+test('#2103: a linked temp name does not take the trim; the log stays a plain file', 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))
+  symlinkSync(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' })
+  const second = await tool('record_decision', { option: 'Trigger it again', choice: 'rejected' })
+
+  assert.equal(readFileSync(target, 'utf8'), before, 'the file behind the temp link is not written through')
+  assert.equal(lstatSync(log).isSymbolicLink(), false, 'the log is still a plain file, not the link')
+  const lines = readFileSync(log, 'utf8').split('\n').filter(Boolean)
+  assert.equal(lines.length, 2_002, 'the refused trim leaves the log as it was, plus the two new lines')
+  for (const lineText of lines) assert.doesNotThrow(() => JSON.parse(lineText), 'every line stays a valid entry')
+  assert.equal(JSON.parse(lines.at(-1)).option, 'Trigger it again')
+  assert.doesNotMatch(first, /^Recorded:/, 'the answer does not claim a clean record when the trim was refused')
+  assert.doesNotMatch(second, /^Recorded:/)
+  assert.equal(warnings.length, 1, 'one warning for the temp name, not one per record')
+  assert.match(warnings[0], /decisions\.jsonl\.tmp/)
+  assert.match(warnings[0], /not a plain file/)
+
+  // The healthy path is untouched: with a normal temp name the trim still bounds the log.
+  const healthy = await boot({ decisions: true })
+  t.after(healthy.stop)
+  const plain = join(healthy.home, 'decisions.jsonl')
+  writeFileSync(plain, full(2_000))
+  const healthyWarnings = []
+  healthy.ctx.logger.warn = (...args) => healthyWarnings.push(args.join(' '))
+  assert.match(await healthy.tool('record_decision', { option: 'Trim normally', choice: 'used' }), /^Recorded:/)
+  const kept = readFileSync(plain, 'utf8').split('\n').filter(Boolean)
+  assert.equal(kept.length, 2_000, 'the trim keeps the newest KEPT lines')
+  assert.equal(JSON.parse(kept.at(-1)).option, 'Trim normally')
+  assert.equal(existsSync(`${plain}.tmp`), false, 'the healthy trim renames its temp name away')
+  assert.deepEqual(healthyWarnings, [])
+})

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. 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.
+- `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. The temp name the housekeeping lands through is asked the same way ([#2103](https://github.com/ThinkFlowLab/FlyBot/issues/2103)): `<home>/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.
 - **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

patches/ds-bot/files/src/host/decisions.js

@@ -44,6 +44,11 @@ export function install(rt) {
   // 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
+  // Housekeeping writes a temp name and renames it over the log, so that name is a write target too
+  // (#2103): a link left there would take the retained log and then become the log. One refusal is
+  // said once, the same way the leaf is.
+  let refusedTemp = false
+  let trimRefused = false
   const leafWritable = async () => {
     if (await fileKind(path) !== 'other') return true
     if (!refusedLeaf) {
@@ -57,6 +62,7 @@ export function install(rt) {
   // file so a reader never sees a half-trimmed log. The Bot side stays append-only; this is the
   // host's own housekeeping.
   async function trim() {
+    trimRefused = false
     let text
     try { text = await readFile(path, 'utf8') } catch (error) { if (error?.code === 'ENOENT') return 0; throw error }
     const lines = text.split('\n').filter(Boolean)
@@ -72,6 +78,14 @@ 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') {
+        trimRefused = true
+        if (!refusedTemp) {
+          refusedTemp = true
+          rt.warn('ds-bot: the decision log was not trimmed: %s is not a plain file', tmp)
+        }
+        return kept.length
+      }
       await writeFile(tmp, next, 'utf8')
       await rename(tmp, path)
     }
@@ -85,7 +99,10 @@ export function install(rt) {
       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()
+      if (linesOnDisk > KEPT || size > MAX_BYTES) {
+        linesOnDisk = await trim()
+        if (trimRefused) return 'untrimmed'
+      }
       return 'written'
     }).catch(error => { rt.warn('ds-bot: a decision was not written: %s', error); return 'failed' })
     return writing
@@ -145,6 +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.`
       return `Recorded: ${choice} "${option}"${why ? ` — ${why}` : ''}.`
     },
   }))

patches/ds-bot/files/test/decisions.test.js

@@ -130,3 +130,53 @@ test('a linked decision log is refused, not written through', async (t) => {
   assert.match(await tool('record_decision', { option: 'Write it now', choice: 'used' }), /^Recorded:/)
   assert.equal(readFileSync(leaf, 'utf8').trim().split('\n').length, 1)
 })
+
+// Housekeeping writes a temp name beside the log and renames it over the log, so the temp name is a
+// place this code writes (#2103). A link left there must be refused before the write, not taken:
+// otherwise the retained log is written through it and the log itself becomes a link.
+test('#2103: a linked temp name does not take the trim; the log stays a plain file', 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))
+  symlinkSync(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' })
+  const second = await tool('record_decision', { option: 'Trigger it again', choice: 'rejected' })
+
+  assert.equal(readFileSync(target, 'utf8'), before, 'the file behind the temp link is not written through')
+  assert.equal(lstatSync(log).isSymbolicLink(), false, 'the log is still a plain file, not the link')
+  const lines = readFileSync(log, 'utf8').split('\n').filter(Boolean)
+  assert.equal(lines.length, 2_002, 'the refused trim leaves the log as it was, plus the two new lines')
+  for (const lineText of lines) assert.doesNotThrow(() => JSON.parse(lineText), 'every line stays a valid entry')
+  assert.equal(JSON.parse(lines.at(-1)).option, 'Trigger it again')
+  assert.doesNotMatch(first, /^Recorded:/, 'the answer does not claim a clean record when the trim was refused')
+  assert.doesNotMatch(second, /^Recorded:/)
+  assert.equal(warnings.length, 1, 'one warning for the temp name, not one per record')
+  assert.match(warnings[0], /decisions\.jsonl\.tmp/)
+  assert.match(warnings[0], /not a plain file/)
+
+  // The healthy path is untouched: with a normal temp name the trim still bounds the log.
+  const healthy = await boot({ decisions: true })
+  t.after(healthy.stop)
+  const plain = join(healthy.home, 'decisions.jsonl')
+  writeFileSync(plain, full(2_000))
+  const healthyWarnings = []
+  healthy.ctx.logger.warn = (...args) => healthyWarnings.push(args.join(' '))
+  assert.match(await healthy.tool('record_decision', { option: 'Trim normally', choice: 'used' }), /^Recorded:/)
+  const kept = readFileSync(plain, 'utf8').split('\n').filter(Boolean)
+  assert.equal(kept.length, 2_000, 'the trim keeps the newest KEPT lines')
+  assert.equal(JSON.parse(kept.at(-1)).option, 'Trim normally')
+  assert.equal(existsSync(`${plain}.tmp`), false, 'the healthy trim renames its temp name away')
+  assert.deepEqual(healthyWarnings, [])
+})