fix(memory): handle zero-initialization in flush compaction dedup#66845
fix(memory): handle zero-initialization in flush compaction dedup#66845wkeything wants to merge 2 commits into
Conversation
When compactionCount is undefined (session never compacted) but memoryFlushCompactionCount is explicitly 0, the dedup check incorrectly reported "already flushed" because undefined ?? 0 matched the flush counter. Now require compactionCount to be an explicit number before comparing, preventing duplicate memory entries on the initial compaction cycle. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Greptile SummaryFixes a real edge case in The new test suite is solid; there are a couple of duplicate cases (see inline comment) that are P2 style cleanup only. Confidence Score: 5/5Safe to merge — the fix is correct, well-explained, and the only remaining findings are duplicate test cases. All findings are P2 (duplicate tests). The logic change itself is straightforward and correct, the comment documents the reasoning, and the test coverage is comprehensive. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/reply-state.test.ts
Line: 367-396
Comment:
**Duplicate test cases**
The two tests at lines 367–374 and 389–396 are identical — both assert that `{ compactionCount: 0, memoryFlushCompactionCount: 0 }` returns `true`. One of them can be removed to avoid confusion about whether there are two distinct scenarios being covered.
Similarly, the tests at lines 376–378 and 380–387 both assert that two `undefined` fields return `false` (the only difference is `{}` vs explicit `undefined` values, which behave the same way for optional fields). Consider collapsing those into a single test.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory): handle zero-initialization ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("returns true when both compactionCount and memoryFlushCompactionCount are explicitly 0", () => { | ||
| expect( | ||
| hasAlreadyFlushedForCurrentCompaction({ | ||
| compactionCount: 0, | ||
| memoryFlushCompactionCount: 0, | ||
| }), | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false when both compactionCount and memoryFlushCompactionCount are undefined (fresh session)", () => { | ||
| expect(hasAlreadyFlushedForCurrentCompaction({})).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when compactionCount is undefined and memoryFlushCompactionCount is undefined", () => { | ||
| expect( | ||
| hasAlreadyFlushedForCurrentCompaction({ | ||
| compactionCount: undefined, | ||
| memoryFlushCompactionCount: undefined, | ||
| }), | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| it("correctly detects flush at cycle 0 when compactionCount is explicit", () => { | ||
| expect( | ||
| hasAlreadyFlushedForCurrentCompaction({ | ||
| compactionCount: 0, | ||
| memoryFlushCompactionCount: 0, | ||
| }), | ||
| ).toBe(true); | ||
| }); |
There was a problem hiding this comment.
The two tests at lines 367–374 and 389–396 are identical — both assert that { compactionCount: 0, memoryFlushCompactionCount: 0 } returns true. One of them can be removed to avoid confusion about whether there are two distinct scenarios being covered.
Similarly, the tests at lines 376–378 and 380–387 both assert that two undefined fields return false (the only difference is {} vs explicit undefined values, which behave the same way for optional fields). Consider collapsing those into a single test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/reply-state.test.ts
Line: 367-396
Comment:
**Duplicate test cases**
The two tests at lines 367–374 and 389–396 are identical — both assert that `{ compactionCount: 0, memoryFlushCompactionCount: 0 }` returns `true`. One of them can be removed to avoid confusion about whether there are two distinct scenarios being covered.
Similarly, the tests at lines 376–378 and 380–387 both assert that two `undefined` fields return `false` (the only difference is `{}` vs explicit `undefined` values, which behave the same way for optional fields). Consider collapsing those into a single test.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b62e6b2b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (typeof entry.compactionCount !== "number") { | ||
| return false; |
There was a problem hiding this comment.
Returning false whenever compactionCount is missing disables flush dedup for legacy/session-store entries that only have memoryFlushCompactionCount=0. In agent-runner-memory.ts, flush persistence still writes memoryFlushCompactionCount using a ?? 0 fallback, so these sessions will continue to look like "already flushed at cycle 0" data but this check now never recognizes it; the result is repeated memory flushes (duplicate MEMORY.md writes and extra model calls) on subsequent turns until a compaction event finally sets compactionCount.
Useful? React with 👍 / 👎.
Addresses Codex review on openclaw#66845: - P1: Restore ?? 0 fallback for compactionCount when memoryFlushCompactionCount is set, preserving backward compat for legacy sessions - P2: Remove duplicate test assertions Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Closing this as implemented after Codex automated review. Current main already implements the final behavior in PR #66845: cycle-0 flush dedup treats missing compactionCount as 0 only when memoryFlushCompactionCount is present, and fresh sessions clear memoryFlushCompactionCount. The PR’s final runtime diff is behaviorally equivalent to main, after the PR discussion restored the compatibility fallback. Best possible solution: Close the PR as already implemented on main. Keep the shipped cycle-0 dedup semantics, where a persisted memoryFlushCompactionCount=0 with missing compactionCount is a valid already-flushed marker, and rely on the existing source/tests unless a new reproduction shows a distinct missed-flush path. What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Codex Review notes: model gpt-5.5, reasoning high; reviewed against d54d2d6b9b8a; fix evidence: release v2026.4.24, commit cbcfdf62c729. |
1 participant
Summary
hasAlreadyFlushedForCurrentCompaction()where zero-initializedmemoryFlushCompactionCountcaused memory flush to be incorrectly skippedmemoryFlushCompactionCountwas explicitly0(from serialization round-trip) butcompactionCountwasundefined(never compacted), the?? 0fallback made both equal, returningtrue("already flushed") even though no flush occurredChanges
?? 0fallback with explicittypeofchecks — both fields must be explicit numbers before comparingundefined, returnsfalse(not yet flushed)Test plan
false(flush needed)0returnstrue(legitimate flush at cycle 0)🤖 Generated with Claude Code