From 616b36c4e8c96a0af1cd0d3c4b5235aad9e6b18d Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Mon, 2 Nov 2026 09:33:35 +0800 Subject: [PATCH] Ask the log's own file before a decision or diagnostics write (#2089) --- .../2089-decisions-diagnostics-leaf.fixed.md | 1 + core/src/host/decisions.js | 22 ++++++++- core/src/host/diagnostics.js | 19 +++++++- core/test/decisions.test.js | 35 +++++++++++++- core/test/diagnostics.test.js | 39 +++++++++++++++- docs/subsystems/core-host.md | 3 +- patches/ds-bot/0001-core-fixes.patch | 38 ++++++++++++++- patches/ds-bot/0007-version-and-tooling.patch | 46 ++++++++++++++++++- patches/ds-bot/files/src/host/decisions.js | 22 ++++++++- patches/ds-bot/files/test/decisions.test.js | 35 +++++++++++++- 10 files changed, 248 insertions(+), 12 deletions(-) create mode 100644 changelog.d/2089-decisions-diagnostics-leaf.fixed.md diff --git a/changelog.d/2089-decisions-diagnostics-leaf.fixed.md b/changelog.d/2089-decisions-diagnostics-leaf.fixed.md new file mode 100644 index 000000000..ca3e02a46 --- /dev/null +++ b/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. diff --git a/core/src/host/decisions.js b/core/src/host/decisions.js index 5c3ff2fc1..21816fa65 100644 --- a/core/src/host/decisions.js +++ b/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}` : ''}.` diff --git a/core/src/host/diagnostics.js b/core/src/host/diagnostics.js index 1465f1794..400c800b5 100644 --- a/core/src/host/diagnostics.js +++ b/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 } diff --git a/core/test/decisions.test.js b/core/test/decisions.test.js index aefdbd2f3..76c200faa 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, 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) +}) diff --git a/core/test/diagnostics.test.js b/core/test/diagnostics.test.js index f46a34ed1..00685b834 100644 --- a/core/test/diagnostics.test.js +++ b/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) => { diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 479ad40e5..7c06ec2a2 100644 --- a/docs/subsystems/core-host.md +++ b/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 `/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 `/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 diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index 26f166e59..7977cb63d 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/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 diff --git a/patches/ds-bot/0007-version-and-tooling.patch b/patches/ds-bot/0007-version-and-tooling.patch index c96f194bf..8f60ca274 100644 --- a/patches/ds-bot/0007-version-and-tooling.patch +++ b/patches/ds-bot/0007-version-and-tooling.patch @@ -795,7 +795,12 @@ diff --git a/scripts/lint.mjs b/scripts/lint.mjs diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js --- a/test/diagnostics.test.js +++ b/test/diagnostics.test.js -@@ -5,7 +5,7 @@ import { syncBuiltinESMExports } from 'node:module' +@@ -1,11 +1,11 @@ + 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' import { join } from 'node:path' import { test } from 'node:test' @@ -861,7 +866,7 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js const again = host(home) assert.deepEqual((await again.diagnostics.entries()).map(entry => entry.code), ['EACCES', 'RATE_LIMIT']) assert.match(await again.diagnostics.logText(), /\n {4}at TestContext/) -@@ -84,6 +84,74 @@ test('warnings are kept, cleaned, written to the log, and read back after a rest +@@ -84,6 +84,111 @@ test('warnings are kept, cleaned, written to the log, and read back after a rest quiet.mock.restore() }) @@ -879,6 +884,43 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js + 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) => { diff --git a/patches/ds-bot/files/src/host/decisions.js b/patches/ds-bot/files/src/host/decisions.js index 5c3ff2fc1..21816fa65 100644 --- a/patches/ds-bot/files/src/host/decisions.js +++ b/patches/ds-bot/files/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}` : ''}.` diff --git a/patches/ds-bot/files/test/decisions.test.js b/patches/ds-bot/files/test/decisions.test.js index aefdbd2f3..76c200faa 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, 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) +})