diff --git a/changelog.d/2103-decisions-trim-temp.fixed.md b/changelog.d/2103-decisions-trim-temp.fixed.md new file mode 100644 index 000000000..27d985df0 --- /dev/null +++ b/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 `/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. diff --git a/core/src/host/decisions.js b/core/src/host/decisions.js index 21816fa65..fe805489a 100644 --- a/core/src/host/decisions.js +++ b/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}` : ''}.` }, })) diff --git a/core/test/decisions.test.js b/core/test/decisions.test.js index 76c200faa..6ae241478 100644 --- a/core/test/decisions.test.js +++ b/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, []) +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 0e265f21c..d340b0fed 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. 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 `/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. - **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/patches/ds-bot/files/src/host/decisions.js b/patches/ds-bot/files/src/host/decisions.js index 21816fa65..fe805489a 100644 --- a/patches/ds-bot/files/src/host/decisions.js +++ b/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}` : ''}.` }, })) diff --git a/patches/ds-bot/files/test/decisions.test.js b/patches/ds-bot/files/test/decisions.test.js index 76c200faa..6ae241478 100644 --- a/patches/ds-bot/files/test/decisions.test.js +++ b/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, []) +})