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 } })