Repository navigation
host: one over-long entry in a memory review answer discards the whole answer and the review never moves past those lines - #2559
Merged
Conversation
Contributor
Author
|
@Birfy 求一次 tip 评审:head
|
Collaborator
|
Verdict: PASS at ca85751 Ran at this tip: Review points:
Not run: |
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 validremove,editoraddbeside it never landed; the cursorthroughis saved only on the success path andqueueReview's catch only warns, so the next turn read the same user lines again with nothing bounding the repeats — session94adfb35-1424-491a-b072-b0765ce9fc39took 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, andremoveis 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-longaddalso 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:
The first difference is the defect itself: the whole answer was discarded, so the valid
removebeside the over-long entry never landed. (Run earlier on49fda03fit failed atmemory.test.js:786:1with the samedoesNotMatchand 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:
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
removelanded, and the warning count stayed at one.Diff against the base
0001-core-fixes.patchis 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/policy37, 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.json 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.shwants a DeepSeek Harness checkout at../deepseek-harness, which this worktree does not have. No client bundle is touched (core/client.jsunchanged,build-client --checkgreen), 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" }