From ca857512a8078593261777f6fd9a056a786b347b Mon Sep 17 00:00:00 2001 From: FlyBot Dev-5 Date: Fri, 6 Nov 2026 01:40:35 +0000 Subject: [PATCH] host: one over-long entry in a memory review answer discards the whole answer and the review never moves past those lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #2558 Owner: Dev-5 Written-by: Dev-5 Reviewer: Birfy Risk tier: T2 — `core/src/host/memory.js`, its test, the generated patch and one changelog fragment. The memory review threw the whole answer away when one entry in it was an entry memory cannot keep (one over 300 characters, say): the throw ran before applyReview, so a valid remove or edit beside it never landed, and the cursor stayed where it was, so the next turn read the same user lines again with nothing bounding the repeats — one session took four of them in a row. One unfit entry is now dropped from the answer with a single warning; the rest still lands, and the cursor moves on. A secret is still dropped silently, and an answer that cannot be read at all (no list of changes, an unlisted entry, a MEMORY.md with no room) fails as before. Docs-not-needed: no architecture change — one branch of the review's own answer handling changes; the modules, the files' shape and what the subsystem page describes are unchanged. --- .../2558-memory-review-unfit-entry.fixed.md | 1 + core/src/host/memory.js | 16 ++-- core/test/memory.test.js | 35 +++++++- patches/ds-bot/0001-core-fixes.patch | 84 ++++++++++++++++++- 4 files changed, 125 insertions(+), 11 deletions(-) create mode 100644 changelog.d/2558-memory-review-unfit-entry.fixed.md diff --git a/changelog.d/2558-memory-review-unfit-entry.fixed.md b/changelog.d/2558-memory-review-unfit-entry.fixed.md new file mode 100644 index 00000000..d9abb01e --- /dev/null +++ b/changelog.d/2558-memory-review-unfit-entry.fixed.md @@ -0,0 +1 @@ +The memory review now leaves out only the one entry it cannot keep and lands the rest of the answer: an over-long entry beside a valid remove or edit used to discard the whole answer and read the same lines again next turn, without a bound on the repeats. The review warns once about what it left out, applies the changes it can, and moves its cursor past those lines. ([#2558](https://github.com/ThinkFlowLab/FlyBot/issues/2558)) diff --git a/core/src/host/memory.js b/core/src/host/memory.js index 22d18cae..eec1014c 100644 --- a/core/src/host/memory.js +++ b/core/src/host/memory.js @@ -1481,15 +1481,21 @@ export function install(rt) { if (changes === undefined) throw new Error(`the model's answer is not a list of changes: ${clip(reply.replace(/\s+/g, ' '), 120)}`) const unknown = changes.find(change => change.op !== 'add' && numbered[Number(change.entry) - 1] === undefined) if (unknown !== undefined) throw new Error(`the model's answer names entry ${unknown.entry}, which is not listed`) - // A secret is left out for good; any other entry memory cannot keep (one too - // long, say) fails the answer, so the next review reads the lines again. + // A secret is left out for good and without a word; any other entry memory + // cannot keep (one too long, say) is left out too, so the rest of the answer + // lands and the next review moves on. One warning, not one per entry. const applied = changes.slice(0, REVIEW_CHANGES_MAX) - const unfit = applied.filter(change => change.op !== 'remove').map(change => cleanEntry(change.text)).find(text => !secretEntry(text) && entryProblem(text) !== undefined) - if (unfit !== undefined) throw new Error(`the model's answer holds an entry memory cannot keep: ${entryProblem(unfit)}`) + const problemOf = change => change.op === 'remove' ? undefined : entryProblem(cleanEntry(change.text)) + const unfit = applied.filter(change => { + const problem = problemOf(change) + return problem !== undefined && !secretEntry(cleanEntry(change.text)) + }) + if (unfit.length > 0) rt.warn('ds-bot: memory review of %s left out an entry memory cannot keep: %s', bot.name, problemOf(unfit[0])) + const landed = applied.filter(change => !unfit.includes(change)) const badTopic = applied.find(change => change.op === 'add' && typeof change.topic === 'string' && change.topic.trim() !== '' && cleanTopic(change.topic) === undefined) if (badTopic !== undefined) throw new Error(`the model's answer names the topic "${clip(badTopic.topic, 60)}", which cannot be a topic name`) if (rt.botOf(bot.id) === undefined) return - const changed = await applyReview(bot, sessionId, numbered, applied, batch.source(read), { teamAllowed: teamAllowedFrom(live, bot) }) + const changed = await applyReview(bot, sessionId, numbered, landed, batch.source(read), { teamAllowed: teamAllowedFrom(live, bot) }) if (rt.botOf(bot.id) === undefined) return // An answer at the limit may have left some out, so the same lines are read // again, a few times at most, while each pass still changes something. diff --git a/core/test/memory.test.js b/core/test/memory.test.js index 135e1583..607875b6 100644 --- a/core/test/memory.test.js +++ b/core/test/memory.test.js @@ -806,18 +806,17 @@ test('an answer that is no list of changes keeps the review\'s place, and the ne assert.deepEqual(JSON.parse(read('reviewed.json')), { 'chat:b2': 11 }) }) -test('an answer with a malformed change, an unlisted entry, an entry too long, or a bad topic changes nothing and keeps the review\'s place', async (t) => { +test('an answer with a malformed change, an unlisted entry, or a bad topic changes nothing and keeps the review\'s place', async (t) => { const replies = [ JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'add' }] }), JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'remove', entry: 7 }] }), - JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'add', text: `The user tells ${'a long story '.repeat(30)}` }] }), JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'add', text: 'The user writes C++', topic: 'C++' }] }), JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'add', text: 'I use hunter2 as my password' }] }), ] const { ctx, call, read, requests, stop } = start({ replies, config: { memoryReviewDelayMs: 0 } }) t.after(() => stop()) await call('state', {}) - for (let tries = 0; tries < 4; tries += 1) { + for (let tries = 0; tries < 3; tries += 1) { ctx.emit('agent/status', { agent: { id: 'b2' }, status: 'idle' }) await reviewed(call) assert.throws(() => read('bots', 'b2', 'MEMORY.md'), { code: 'ENOENT' }) @@ -826,11 +825,39 @@ test('an answer with a malformed change, an unlisted entry, an entry too long, o ctx.emit('agent/status', { agent: { id: 'b2' }, status: 'idle' }) await reviewed(call) // A password is only left out: the rest is written and the place moves on. - assert.equal(requests.length, 5) + assert.equal(requests.length, 4) assert.deepEqual(parseMemory(read('bots', 'b2', 'MEMORY.md')).entries.map(entry => entry.text), ['The user likes tea']) assert.deepEqual(JSON.parse(read('reviewed.json')), { 'chat:b2': 11 }) }) +test('an entry memory cannot keep is left out, the rest of the answer lands, and the review moves on', async (t) => { + const tooLong = `The user tells ${'a long story '.repeat(25)}and more` + assert.equal(tooLong.length, 348) + const warned = [] + const original = console.error + console.error = (...args) => { warned.push(args.join(' ')) } + t.after(() => { console.error = original }) + const replies = [JSON.stringify({ changes: [{ op: 'add', text: tooLong }, { op: 'remove', entry: 1 }] })] + const { tool, ctx, call, read, requests, stop } = start({ replies, config: { memoryReviewDelayMs: 0 } }) + t.after(() => stop()) + await call('state', {}) + await tool('remember', { text: 'Likes tea' }, 'b2') + ctx.emit('agent/status', { agent: { id: 'b2' }, status: 'idle' }) + await reviewed(call) + assert.equal(requests.length, 1) + // The remove landed even though the entry beside it was one memory cannot keep. + assert.doesNotMatch(read('bots', 'b2', 'MEMORY.md'), /Likes tea/) + assert.doesNotMatch(read('bots', 'b2', 'MEMORY.md'), /a long story/) + // Left out once, and said so once: the answer was not failed as a whole. + assert.equal(warned.filter(line => /memory review of .* cannot keep/.test(line)).length, 1, warned.join('\n')) + assert.equal(warned.filter(line => /memory review of .* failed/.test(line)).length, 0, warned.join('\n')) + // The cursor moved past the line it read, so the next review calls no model at all. + ctx.emit('agent/status', { agent: { id: 'b2' }, status: 'idle' }) + await reviewed(call) + assert.equal(requests.length, 1) + assert.deepEqual(JSON.parse(read('reviewed.json')), { 'chat:b2': 11 }) +}) + test('a backlog past the text limit is reviewed in passes, oldest first', async (t) => { const home = mkdtempSync(join(tmpdir(), 'ds-bot-memory-')) mkdirSync(join(home, 'memory'), { recursive: true }) diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index 81c97726..ae979999 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -6290,12 +6290,30 @@ diff --git a/src/host/memory.js b/src/host/memory.js const earlier = batch.earlier.map(line => reviewLine(line.who, line.text, bot)) const language = writtenIn(read.filter(line => line.who === 'user').map(line => line.text.replace(/^User: /, '')), languageNow()) const reply = await botShot(bot, reviewSystem(writesTeam(bot.id)), reviewRequest({ bot, numbered, left, sizes, where: batch.where, earlier, fresh: fresh.texts, more, language }), { sessionId: rt.chatOf(bot.id), kind: 'memory' }) -@@ -1272,7 +1272,7 @@ export function install(rt) { +@@ -1264,15 +1264,21 @@ export function install(rt) { + if (changes === undefined) throw new Error(`the model's answer is not a list of changes: ${clip(reply.replace(/\s+/g, ' '), 120)}`) + const unknown = changes.find(change => change.op !== 'add' && numbered[Number(change.entry) - 1] === undefined) + if (unknown !== undefined) throw new Error(`the model's answer names entry ${unknown.entry}, which is not listed`) +- // A secret is left out for good; any other entry memory cannot keep (one too +- // long, say) fails the answer, so the next review reads the lines again. ++ // A secret is left out for good and without a word; any other entry memory ++ // cannot keep (one too long, say) is left out too, so the rest of the answer ++ // lands and the next review moves on. One warning, not one per entry. + const applied = changes.slice(0, REVIEW_CHANGES_MAX) +- const unfit = applied.filter(change => change.op !== 'remove').map(change => cleanEntry(change.text)).find(text => !secretEntry(text) && entryProblem(text) !== undefined) +- if (unfit !== undefined) throw new Error(`the model's answer holds an entry memory cannot keep: ${entryProblem(unfit)}`) ++ const problemOf = change => change.op === 'remove' ? undefined : entryProblem(cleanEntry(change.text)) ++ const unfit = applied.filter(change => { ++ const problem = problemOf(change) ++ return problem !== undefined && !secretEntry(cleanEntry(change.text)) ++ }) ++ if (unfit.length > 0) rt.warn('ds-bot: memory review of %s left out an entry memory cannot keep: %s', bot.name, problemOf(unfit[0])) ++ const landed = applied.filter(change => !unfit.includes(change)) const badTopic = applied.find(change => change.op === 'add' && typeof change.topic === 'string' && change.topic.trim() !== '' && cleanTopic(change.topic) === undefined) if (badTopic !== undefined) throw new Error(`the model's answer names the topic "${clip(badTopic.topic, 60)}", which cannot be a topic name`) if (rt.botOf(bot.id) === undefined) return - const changed = await applyReview(bot, sessionId, numbered, applied, batch.source(read)) -+ const changed = await applyReview(bot, sessionId, numbered, applied, batch.source(read), { teamAllowed: teamAllowedFrom(live, bot) }) ++ const changed = await applyReview(bot, sessionId, numbered, landed, batch.source(read), { teamAllowed: teamAllowedFrom(live, bot) }) if (rt.botOf(bot.id) === undefined) return // An answer at the limit may have left some out, so the same lines are read // again, a few times at most, while each pass still changes something. @@ -12366,6 +12384,68 @@ diff --git a/test/memory.test.js b/test/memory.test.js test('when a turn ends, the memory review keeps what the user said, through the same rules', async (t) => { const history = { b2: [ +@@ -652,18 +652,17 @@ test('an answer that is no list of changes keeps the review\'s place, and the ne + assert.deepEqual(JSON.parse(read('reviewed.json')), { 'chat:b2': 11 }) + }) + +-test('an answer with a malformed change, an unlisted entry, an entry too long, or a bad topic changes nothing and keeps the review\'s place', async (t) => { ++test('an answer with a malformed change, an unlisted entry, or a bad topic changes nothing and keeps the review\'s place', async (t) => { + const replies = [ + JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'add' }] }), + JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'remove', entry: 7 }] }), +- JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'add', text: `The user tells ${'a long story '.repeat(30)}` }] }), + JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'add', text: 'The user writes C++', topic: 'C++' }] }), + JSON.stringify({ changes: [{ op: 'add', text: 'The user likes tea' }, { op: 'add', text: 'I use hunter2 as my password' }] }), + ] + const { ctx, call, read, requests, stop } = start({ replies, config: { memoryReviewDelayMs: 0 } }) + t.after(() => stop()) + await call('state', {}) +- for (let tries = 0; tries < 4; tries += 1) { ++ for (let tries = 0; tries < 3; tries += 1) { + ctx.emit('agent/status', { agent: { id: 'b2' }, status: 'idle' }) + await reviewed(call) + assert.throws(() => read('bots', 'b2', 'MEMORY.md'), { code: 'ENOENT' }) +@@ -672,11 +672,39 @@ test('an answer with a malformed change, an unlisted entry, an entry too long, o + ctx.emit('agent/status', { agent: { id: 'b2' }, status: 'idle' }) + await reviewed(call) + // A password is only left out: the rest is written and the place moves on. +- assert.equal(requests.length, 5) ++ assert.equal(requests.length, 4) + assert.deepEqual(parseMemory(read('bots', 'b2', 'MEMORY.md')).entries.map(entry => entry.text), ['The user likes tea']) + assert.deepEqual(JSON.parse(read('reviewed.json')), { 'chat:b2': 11 }) + }) + ++test('an entry memory cannot keep is left out, the rest of the answer lands, and the review moves on', async (t) => { ++ const tooLong = `The user tells ${'a long story '.repeat(25)}and more` ++ assert.equal(tooLong.length, 348) ++ const warned = [] ++ const original = console.error ++ console.error = (...args) => { warned.push(args.join(' ')) } ++ t.after(() => { console.error = original }) ++ const replies = [JSON.stringify({ changes: [{ op: 'add', text: tooLong }, { op: 'remove', entry: 1 }] })] ++ const { tool, ctx, call, read, requests, stop } = start({ replies, config: { memoryReviewDelayMs: 0 } }) ++ t.after(() => stop()) ++ await call('state', {}) ++ await tool('remember', { text: 'Likes tea' }, 'b2') ++ ctx.emit('agent/status', { agent: { id: 'b2' }, status: 'idle' }) ++ await reviewed(call) ++ assert.equal(requests.length, 1) ++ // The remove landed even though the entry beside it was one memory cannot keep. ++ assert.doesNotMatch(read('bots', 'b2', 'MEMORY.md'), /Likes tea/) ++ assert.doesNotMatch(read('bots', 'b2', 'MEMORY.md'), /a long story/) ++ // Left out once, and said so once: the answer was not failed as a whole. ++ assert.equal(warned.filter(line => /memory review of .* cannot keep/.test(line)).length, 1, warned.join('\n')) ++ assert.equal(warned.filter(line => /memory review of .* failed/.test(line)).length, 0, warned.join('\n')) ++ // The cursor moved past the line it read, so the next review calls no model at all. ++ ctx.emit('agent/status', { agent: { id: 'b2' }, status: 'idle' }) ++ await reviewed(call) ++ assert.equal(requests.length, 1) ++ assert.deepEqual(JSON.parse(read('reviewed.json')), { 'chat:b2': 11 }) ++}) ++ + test('a backlog past the text limit is reviewed in passes, oldest first', async (t) => { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-memory-')) + mkdirSync(join(home, 'memory'), { recursive: true }) @@ -994,7 +994,7 @@ test('a turn whose Session moves to a fresh part before its review is still revi test('config.memoryReview false leaves memory to the Bots\' own remember calls', async (t) => { const { ctx, call, section, requests, stop } = start({ config: { memoryReview: false, memoryReviewDelayMs: 0 } })