fix(lcm-recent): preserve historical rollups during pending rebuild#29
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughSanitizes “message:” fallback evidence to only include ChangesFallback Filtering and Pending Rollup Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 59 minutes and 36 seconds. Comment |
There was a problem hiding this comment.
Pull request overview
This PR narrows the lcm_recent “pending rebuild” freshness gate so that historical day/week/month requests can still serve ready stored rollups when the pending activity (last_message_at) is outside the requested window, while keeping conservative fallback behavior for current-day and unknown-bounds cases.
Changes:
- Refines pending-rebuild gating to only bypass stored rollups when the pending activity intersects the requested window (or when bounds are unknown).
- Hardens bounded raw-message fallback to exclude
tool/systemroles and internal context markers from being surfaced. - Updates/extends tests to cover historical rollup preservation during pending rebuilds and sanitized fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/tools/lcm-recent-tool.ts |
Tightens pending-rebuild window detection and filters raw fallback messages to avoid leaking tool/system/internal context. |
test/rollup-store-builder.test.ts |
Adds coverage for sanitized fallback and verifies stored historical rollups remain usable when pending rebuild is for later/out-of-window activity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1155
to
+1158
| AND m.role IN ('user', 'assistant') | ||
| AND instr(m.content, '<<<BEGIN_OPENCLAW_INTERNAL_CONTEXT>>>') = 0 | ||
| AND instr(m.content, '<<<END_OPENCLAW_INTERNAL_CONTEXT>>>') = 0 | ||
| AND instr(m.content, '[lossless-claw] missing tool result') = 0 |
Comment on lines
1500
to
+1504
| resolution.periodKey == null || resolution.periodKey !== currentDayKey; | ||
| const hasPendingRebuild = rollupState?.pending_rebuild === 1; | ||
| const pendingRebuildUnknown = hasPendingRebuild && lastPendingMessageAt == null; | ||
| const pendingRebuildTouchesWindow = | ||
| hasPendingRebuild && (resolution.kind === "day" || pendingDayKey != null); | ||
| hasPendingRebuild && (pendingRebuildUnknown || pendingDayKey != null); |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/lcm-recent-tool.ts`:
- Around line 1502-1506: The code currently treats an unparseable
last_message_at (resulting in an invalid Date in lastPendingMessageAt) as if it
were present, making pendingRebuildUnknown false; update the logic to treat
invalid Dates as unknown by checking for NaN/getTime() before using
lastPendingMessageAt. For example, normalize lastPendingMessageAt or change the
pendingRebuildUnknown computation to use hasPendingRebuild &&
(lastPendingMessageAt == null || isNaN(lastPendingMessageAt.getTime())) so
pendingRebuildTouchesWindow and canUseStoredResolvedRollup behave correctly when
last_message_at is unparseable.
- Around line 1585-1589: The branch that returns stored rollups for
resolution.kind when resolution.window exists incorrectly allows using stored
weekly/monthly aggregates even if the pending rebuild overlaps that window;
update the condition that now reads "resolution.kind && !resolution.window &&
!pendingRebuildUnknown" to also exclude cases where the pending rebuild touches
the resolution.window (e.g., check pendingDayKey or the pending rebuild range
against resolution.window). Concretely, add an overlap check (use an existing
helper like pendingRebuildContains or add a small isWithinWindow(pendingDayKey,
resolution.window) check) and change the condition to require no overlap (e.g.,
&& !pendingRebuildUnknown && !pendingRebuildOverlapsWindow) so touched aggregate
windows fall through to fallback instead of returning stored rollups.
- Around line 1155-1158: The count/coverage subqueries (notably availableCount
and hasFallbackSourceItemsInRange()) are still counting unsanitized rows
(tool/system/internal and messages containing the internal markers) while the
fetch query filters them out, causing totalMatches inflation; update those
downstream message-counting subqueries to apply the exact same sanitized
predicate used in the fetch (i.e., AND m.role IN ('user','assistant') AND
instr(m.content,'<<<BEGIN_OPENCLAW_INTERNAL_CONTEXT>>>') = 0 AND
instr(m.content,'<<<END_OPENCLAW_INTERNAL_CONTEXT>>>') = 0 AND
instr(m.content,'[lossless-claw] missing tool result') = 0) so availableCount,
hasFallbackSourceItemsInRange(), and any other message-counting expressions use
consistent filtering with the fetch path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b0d4110-ee02-41f4-9377-99a798aff68f
📒 Files selected for processing (2)
src/tools/lcm-recent-tool.tstest/rollup-store-builder.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: test
- GitHub Check: test
added 2 commits
May 4, 2026 23:28
- C1 (P0) Sanitized message predicate now consistently applied in all 3
query sites (availableCount, hasFallbackSourceItemsInRange, fetch).
Extracted SANITIZED_FALLBACK_INTERNAL_MARKERS + buildSanitizedMessagePredicate
so role + internal-marker filters can never drift between fetch and
count/coverage paths.
- C2 (P0) Added pendingRebuildTouchesWindow gate to weekly/monthly stored
rollup branch. Day branch already covered by per-day usableKeys filter.
- M1 (P1) Treat unparseable last_message_at as unknown bounds (Number.isNaN
check at the source), so corrupt rollup state cannot silently serve stale
data via Invalid Date that compares != null.
- M2 (P2) Centralized internal-marker filter list in
SANITIZED_FALLBACK_INTERNAL_MARKERS; new markers go in one place.
- 3 regression tests:
* "ignores tool/system/internal raw rows when probing fallback presence"
(C1: stored rollup wins over harness-only-fallback signal)
* "bypasses stored weekly rollup when pending rebuild touches the window"
(C2: weekly aggregate falls through to fallback when pendingDayKey lands
inside it)
* "treats unparseable last_message_at as unknown pending bounds"
(M1: corrupt timestamp -> unknown-bounds degraded reason, stale rollup
bypassed)
74e8b97 to
8c56c0a
Compare
This was referenced May 4, 2026
Merged
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.
Summary
Fixes
lcm_recentserving fallback instead of stored historical rollups wheneverpending_rebuild=1.The read-path freshness gate was too broad: any explicit day request was treated as unsafe while rebuild was pending, even if
last_message_atwas outside the requested historical date/window. This made closed dates likedate:2026-05-02bypass ready stored rollups after current-day activity dirtied the conversation.Policy
system/toolmessages and obvious OpenClaw internal context markers.Bug report
Issues are disabled on this repo, so filing the bug as this PR instead.
Repro
lcm_rollup_state.pending_rebuild=1andlast_message_atpoints to today/current window.lcm_recent({ period: "date:<prior closed date>" }).Before: response was fallback with “Rollup rebuild is pending…” even though a ready stored day rollup existed.
After: response serves the ready stored historical rollup.
Verification
Local results:
Summary by CodeRabbit
Refactor
Tests