Skip to content

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

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/5-memory-review-unfit-entry
Nov 6, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/5-memory-review-unfit-entry

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

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.

What changed

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, edit or add beside it never landed; the cursor through is saved only on the success path and queueReview's catch only warns, so the next turn read the same user lines again with nothing bounding the repeats — session 94adfb35-1424-491a-b072-b0765ce9fc39 took four in a row (303, 561, 308 and 358 characters).

Now that one entry is dropped from the answer alone: the review warns once (memory review of <bot> left out an entry memory cannot keep: …), applies the rest of the answer, and moves its cursor past those lines. A secret is still dropped silently and never warned about, and remove is never filtered. An answer that cannot be read at all — no list of changes, an unlisted entry, a bad topic, a MEMORY.md with no room — fails exactly as before.

One existing case changes with it, and this is the part most likely to be questioned. In core/test/memory.test.js, "an answer with a malformed change, an unlisted entry, or a bad topic changes nothing and keeps the review's place" carried a fourth leg asserting that an over-long add also landed nothing and left the cursor where it was — precisely the semantics being replaced. That leg moved into the new case as the deliberate control: the over-long entry must not reach MEMORY.md. The other four legs (malformed, unlisted entry, bad topic, password) stay in place with their assertions unchanged.

Red first

The new case, run on the base with only the test file applied:

not ok 33 - an entry memory cannot keep is left out, the rest of the answer lands, and the review moves on
  location: 'core/test/memory.test.js:833:1'
  error: |-
    The input was expected to not match the regular expression /Likes tea/. Input:

    '# Memory\n\n- Likes tea [source: part 1 #10; added: 2026-11-06]\n\n## Index\n'

  code: 'ERR_ASSERTION'
  name: 'AssertionError'
  operator: 'doesNotMatch'
# tests 100 / # pass 99 / # fail 1

The first difference is the defect itself: the whole answer was discarded, so the valid remove beside the over-long entry never landed. (Run earlier on 49fda03f it failed at memory.test.js:786:1 with the same doesNotMatch and the same actual value; #2557 then shifted the line numbers and added a case above it.)

Readings from the same case, with a probe printing the observable state before and after the fix:

base:   {"requestsAfterFirst":1,"requestsAfterSecond":2,"secondPromptHoldsSameUserLine":true,"cursor":{"chat:b2":9},"memoryHoldsTea":true,"memoryHoldsLongEntry":false,"warnedCannotKeep":1,"warnedFailed":2}
fixed:  {"requestsAfterFirst":1,"requestsAfterSecond":1,"secondPromptHoldsSameUserLine":null,"cursor":{"chat:b2":11},"memoryHoldsTea":false,"memoryHoldsLongEntry":false,"warnedCannotKeep":1,"warnedFailed":0}

Before: the next idle turn wrote the same lines into a second prompt and the cursor stayed at 9. After: no second review, no second prompt, the cursor moved to 11, the valid remove landed, and the warning count stayed at one.

Diff against the base

 core/src/host/memory.js                    | 16 +++++-----
 core/test/memory.test.js                   | 35 +++++++++++++++++----
 patches/ds-bot/0001-core-fixes.patch       | 84 ++++++++++++++++++++++++++++++-
 changelog.d/2558-memory-review-unfit-entry.fixed.md |  1 +
 4 files changed, 125 insertions(+), 11 deletions(-)

0001-core-fixes.patch is regenerated (node scripts/ds-bot-patches.mjs generate), not hand-written.

Verification at the tip

Tip SHA: ca85751

Ran:

  • TZ=UTC ARCH_DOCS_BASE=c907f460 npm run verify → exit 0: core 2945/2945, plugins social 577, tree 137, timeline 144, workflow 441, .github/policy 37, interop 26, all pass; arch-docs: OK (4 changed file(s)); patches/ds-bot/ is what generate writes (generate is a no-op); 10 patches apply cleanly in order.
  • cd core && TZ=UTC node --import ./test/harness-resolve.mjs --test test/memory.test.js on the base with only the test file: 100 tests, 99 pass, 1 fail (the case above).
  • node .github/scripts/changelog.mjs check → 4 fragment(s) in changelog.d/ are well formed.

Not run (and why): the bench smoke test — dev/bench.sh wants a DeepSeek Harness checkout at ../deepseek-harness, which this worktree does not have. No client bundle is touched (core/client.js unchanged, build-client --check green), and the changed behaviour is covered at the prompt and cursor level by the unit case.

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.

Checkpoint

{
  "pr": 2559,
  "claim": "Dev-5",
  "issue": 2558,
  "summary": "The memory review no longer throws a whole answer away because one entry in it is over the length limit. That one entry is dropped with a single warning, the rest of the answer — a remove, an edit, an add — lands, and the cursor moves past the lines that were read. A secret is still dropped silently, a remove is never filtered, and an answer that cannot be read at all still fails as before. The existing malformed-answer case keeps its other four legs; its over-long leg moved into the new case as the control.",
  "files": [
    "core/src/host/memory.js",
    "core/test/memory.test.js",
    "patches/ds-bot/0001-core-fixes.patch",
    "changelog.d/2558-memory-review-unfit-entry.fixed.md"
  ],
  "checks": {
    "ran": "TZ=UTC ARCH_DOCS_BASE=c907f460 npm run verify; node scripts/ds-bot-patches.mjs generate && check; node .github/scripts/changelog.mjs check",
    "result": "green",
    "at": "ca857512a8078593261777f6fd9a056a786b347b"
  },
  "next": "none — ready for review"
}

@HugoXK

HugoXK commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy 求一次 tip 评审:head ca857512a8078593261777f6fd9a056a786b347b(本条是该 tip 的唯一一次点名)。

  • 面:core/src/host/memory.js 一处 —— 受理一份 review 答案时,原来 find + throw(main 上 :1486-1487)把「有一条 memory 留不下的条目」当成整份失败,throw 在 applyReview 之前,所以同答案里合法的 remove / edit / add 一条都不落地;游标 through 只在成功路径保存,queueReview 的 catch 只 warn,于是下一 turn 又读同一批 user 行,这条路径没有上限(REVIEW_PASSES_MAX 只管溢出分批)。现在改成过滤:丢掉那一条、只 warn 一次、其余交给 applyReview。秘密条目仍静默丢弃,remove 永不过滤;parse 失败 / unlisted entry / bad topic / MEMORY.md 满 保持原样失败。core/src/host/text.js 零改动(clip() 本体不碰,不开 client 车道)。
  • 单:host: one over-long entry in a memory review answer discards the whole answer and the review never moves past those lines #2558。base c907f460(= 当前 main,无需 rebase)。
  • 红先:clean base + 只放本 PR 的 core/test/memory.test.js → not ok 33 - an entry memory cannot keep is left out, the rest of the answer lands, and the review moves on,location: core/test/memory.test.js:833:1,doesNotMatch /Likes tea/,# tests 100 / pass 99 / fail 1。第一处差异就是缺陷本身:整份被扔,旁边合法的 remove 没落地。
  • 读数(同一用例探针):base {"requestsAfterFirst":1,"requestsAfterSecond":2,"secondPromptHoldsSameUserLine":true,"cursor":{"chat:b2":9},"memoryHoldsTea":true,"warnedFailed":2} → fixed {"requestsAfterFirst":1,"requestsAfterSecond":1,"secondPromptHoldsSameUserLine":null,"cursor":{"chat:b2":11},"memoryHoldsTea":false,"warnedCannotKeep":1,"warnedFailed":0}:游标 9→11、remove 落地、超长条目不进 MEMORY.md、只 warn 一次。
  • 生产证据:同一 session 连续四次 review 都倒在这条上(303 / 561 / 308 / 358 字符),每条都扔掉整份答案。
  • 最可能被问的一点,写在正文里了:core/test/memory.test.js 既有用例「an answer with a malformed change, an unlisted entry, or a bad topic changes nothing and keeps the review's place」原本还有第四条腿,断言超长 add 同样整份不落地、游标不动——那正是被替换掉的旧语义,所以它搬进新用例当对照;其 malformed / unlisted / bad topic / password 四条原地不动,没有放宽断言。
  • 门(tip 实读):TZ=UTC ARCH_DOCS_BASE=c907f460 npm run verify exit 0,core 2945/2945,arch-docs: OK (4 changed file(s)),10 patches apply cleanly in order,canonical → generate is a no-op,changelog 4 片段 well formed;0001-core-fixes.patch 是 generate 重生成,不是手写。
  • Checkpoint:用 live main 的 .github/scripts/enqueue-rules.mjs 在本机跑 checkpointReasons → [];正文无占位符。
  • tier:T2(core/src/host/**),因此需要点名当前 tip 的 Verdict: PASS 才能入队。

@Birfy

Birfy commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at ca85751
Written-by: Reviewer
Attestation: ca85751 role=Reviewer

Ran at this tip:

$ cd core && TZ=UTC node --import ./test/harness-resolve.mjs --test test/memory.test.js
# tests 100  # pass 100  # fail 0
$ red check: same tip tree with core/src/host/memory.js reverted to the base blob c907f460, same command
# tests 100  # pass 99  # fail 1

Review points:

  • Red is the defect itself. With the source reverted the only failure is not ok 33 - an entry memory cannot keep is left out, the rest of the answer lands, and the review moves on, whose first difference is the whole-answer discard (doesNotMatch /Likes tea/). Green restores it; no other case changes.
  • Fix shape (core/src/host/memory.js:1484-1497): unfit entries are filtered before applyReview (landed), one warning naming the first reason; secrets stay silent, remove is never filtered, and parse failure / unlisted entry / bad topic / MEMORY.md-full still throw as before. Cursor moves on the success path that now exists for these lines, so the repeated-review loop from session 94adfb35… ends.
  • The one test-semantics change is honest. memory.test.js:809 keeps its malformed / unlisted / bad-topic / password legs with unchanged assertions; the over-long leg moved into the new case at :833 as the control (doesNotMatch /a long story/, remove lands, one cannot keep warning, no second model call, cursor chat:b2 9→11).
  • Scope: base is current main c907f4605ff0; diff is 4 files, all in the change (memory.js, its test, regenerated patches/ds-bot/0001-core-fixes.patch, one changelog fragment). patches/ds-bot is generated, not hand-written.
  • Tip check-runs: tests-ok, merge-gate (report-only), risk tier, both plugin suites, package and the node 24 job are success; the only red is enqueue-guard with no Verifier report names the tip ca857512a807 — which this PASS supplies. Fork: issue reference and agent-pr are not asked of it.
  • Checkpoint block is present and its fields match this PR (pr 2559, claim Dev-5, issue 2558, 4 files, green at this sha).

Not run: npm run verify end to end and the bench smoke; tests-ok at this tip already covers the full verify order, and no client bundle or host entry point is touched.

@github-actions github-actions Bot added the pass label Nov 6, 2026
@Birfy
Birfy added this pull request to the merge queue Nov 6, 2026
Merged via the queue into ThinkFlowLab:main with commit 50df2d3 Nov 6, 2026
15 checks passed
@Birfy
Birfy deleted the dev/5-memory-review-unfit-entry branch November 6, 2026 01:38
…e answer and the review never moves past those lines

Fixes ThinkFlowLab#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants