host: one over-long entry in a memory review answer discards the whole answer and the review never moves past those lines

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

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.

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

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