Filter internal compaction artifacts from chat history#70348
Filter internal compaction artifacts from chat history#70348cholaolu-boop wants to merge 1 commit into
Conversation
Greptile SummaryThis PR filters three categories of internal compaction artifacts from Confidence Score: 5/5Safe to merge; filtering logic is correct, well-tested, and isolated to the sanitization layer. All findings are P2. The only observation is a redundant second call to No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/server-methods/chat.ts
Line: 1037-1040
Comment:
**Redundant post-sanitization drop check**
`sanitizeChatHistoryMessage` only truncates/strips envelope content — it never adds an `__openclaw.kind` field or rewrites text to begin with a compaction prefix, so any message already passing the pre-sanitization guard at line 1027 will never flip to matching `shouldDropInternalHistoryMessage` after processing. This second invocation is dead code in practice and could mislead future readers into thinking sanitization can manufacture internal markers.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Filter internal compaction artifacts fro..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c520a62240
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| text.startsWith("Pre-compaction memory flush.") || | ||
| text.startsWith("[Post-compaction context refresh]") |
There was a problem hiding this comment.
Restrict internal-history filtering to synthetic artifacts
The new prefix-based filter drops any history message whose text starts with Pre-compaction memory flush. or [Post-compaction context refresh] regardless of role or provenance, so legitimate user/assistant content that begins with those phrases is now silently removed from chat.history (and from session-history snapshots that reuse this sanitizer). This introduces false-positive data loss; the filter should be constrained to synthetic/internal entries (for example via metadata and/or stricter control-token checks) instead of raw text prefix alone.
Useful? React with 👍 / 👎.
| if (shouldDropInternalHistoryMessage(res.message)) { | ||
| changed = true; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
sanitizeChatHistoryMessage only truncates/strips envelope content — it never adds an __openclaw.kind field or rewrites text to begin with a compaction prefix, so any message already passing the pre-sanitization guard at line 1027 will never flip to matching shouldDropInternalHistoryMessage after processing. This second invocation is dead code in practice and could mislead future readers into thinking sanitization can manufacture internal markers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/chat.ts
Line: 1037-1040
Comment:
**Redundant post-sanitization drop check**
`sanitizeChatHistoryMessage` only truncates/strips envelope content — it never adds an `__openclaw.kind` field or rewrites text to begin with a compaction prefix, so any message already passing the pre-sanitization guard at line 1027 will never flip to matching `shouldDropInternalHistoryMessage` after processing. This second invocation is dead code in practice and could mislead future readers into thinking sanitization can manufacture internal markers.
How can I resolve this? If you propose a fix, please make it concise.|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open: the bug area is valid, but this branch is not merge-ready because it drops the documented compaction checkpoint divider, uses provenance-free prefix filtering, is dirty against current main, and lacks real behavior proof. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes, source-level: current main converts transcript compaction records into Is this the best way to solve the issue? No. The safer fix is provenance-aware filtering in the current shared display projection, not unconditional removal of compaction markers or text-prefix rows. Security review: Security review cleared: The diff changes Gateway TypeScript and tests only; no dependency, workflow, permission, secret-handling, or supply-chain concern was found. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against fa614d0907e8. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
Summary
This patch addresses the dominant
chat.historyvisibility path where internal compaction and memory-flush artifacts leak back into normal chat history.What This Fix Does
chat.historyPre-compaction memory flush.prompts fromchat.history[Post-compaction context refresh]artifacts fromchat.historyExplicitly Not Part Of This Fix
audio payload removed during overflow recoveryis not part of this patchWhy This Scope
The patch is intentionally limited to the dominant
chat.history/ compaction / flush visibility path. It does not touch Route A/B, step 1b, step 2, approval, heartbeat, or think-level behavior.Optional Follow-up