fix(memory): skip placeholder short-term promotions#82028
Conversation
|
Codex review: needs changes before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. Current main has a source-level reproduction path because non-empty markdown placeholders pass the existing empty/dreaming-contamination filters into ranking/apply; I did not run tests because this review is read-only. PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge Security Review findings
Review detailsBest possible solution: Centralize the promotion guard, strip trailing colons from known skeleton labels with regression coverage, and merge only after maintainers accept pruning known template headings from persisted short-term state. Do we have a high-confidence way to reproduce the issue? Yes. Current main has a source-level reproduction path because non-empty markdown placeholders pass the existing empty/dreaming-contamination filters into ranking/apply; I did not run tests because this review is read-only. Is this the best way to solve the issue? No, not quite. The centralized guard is the right shape, but the current predicate misses trailing-colon known skeleton headings, so a narrow normalization/test repair is needed before merge. Label changes:
Label justifications:
Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against c8a35c4645dc. |
c36f825 to
56ca6dc
Compare
Added real behavior proof |
|
@hxy91819 PTAL thanks! |
e85b15e to
4e8b549
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e8b549f86
ℹ️ 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".
18d74b6 to
be5a837
Compare
705b245 to
a7a7e7e
Compare
a7a7e7e to
8b3146c
Compare
e0788a9 to
6c49f02
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c49f02b48
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f9bf5f323
ℹ️ 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".
| if (/:\s*\S/.test(headingBody) || headingWords.length >= 4) { | ||
| return false; |
There was a problem hiding this comment.
Allow concise heading snippets to be promoted
isMarkdownSkeletonSnippet currently classifies heading lines as placeholders unless they contain a colon or at least 4 words, so concise but durable notes like ## API keys rotated or - ## VPN outage fix are treated as unpromotable. Because isUnpromotableShortTermSnippet now gates store normalization, recording, ranking, and apply paths, these legitimate short-term memories are silently dropped and never make it into MEMORY.md.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f3c21510a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 545febdb1e
ℹ️ 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".
|
|
||
| function normalizeMarkdownSkeletonHeadingBody(raw: string): string { | ||
| return raw | ||
| .replace(/[-*+>`_[\](){}|~.,;!?]+/g, " ") |
There was a problem hiding this comment.
Strip trailing colons from skeleton heading labels
In normalizeMarkdownSkeletonHeadingBody, the punctuation cleanup omits :, so a placeholder heading like ## Notes: normalizes to notes: instead of notes and misses MARKDOWN_SKELETON_HEADING_BODIES; isMarkdownSkeletonSnippet then treats it as promotable content. Because this guard now gates store normalization, recording, ranking, and apply paths, common template headings with trailing colons can still be promoted into MEMORY.md, which undermines the placeholder filter this commit introduces.
Useful? React with 👍 / 👎.
Summary
Fixes #80582.
Verification
node scripts/run-vitest.mjs extensions/memory-core/src/short-term-promotion.test.ts -- --runInBandnode scripts/run-vitest.mjs extensions/memory-core/src/dreaming-phases.test.ts extensions/memory-core/src/short-term-promotion.test.ts -- --runInBandCI=true pnpm exec oxfmt --check --threads=1 extensions/memory-core/src/short-term-promotion.ts extensions/memory-core/src/short-term-promotion.test.tsgit diff --checkReal behavior proof
Behavior addressed: Short-term promotion could treat markdown placeholders like
-, plain heading-only scaffolding like## Tagesnotizen ## Entscheidungen, trailing-colon scaffold headings like## Notes:, and previously persisted normalized marker-plus-heading snippets like- ## Entscheidungenas durable memory candidates, allowing empty daily-note scaffolding to rank and eventually append toMEMORY.md. The guard also preserves list-wrapped heading content with durable body text, including localized non-colon headings such as- ## 修复备份轮换.Real environment tested: Local filesystem-backed OpenClaw memory workspace on macOS, using PR head
519ee06236eca61c78764f59fa1837157efb37bf. The standalone Node run created real temp files undermemory/, wrotememory/.dreams/short-term-recall.json, called the productionrankShortTermPromotionCandidatesandapplyShortTermPromotionsfunctions fromextensions/memory-core/src/short-term-promotion.ts, and checked the resultingMEMORY.mdfilesystem state.Exact steps or command run after this patch: From the repo root, ran
node --import tsx -with a standalone script that seeds temp workspaces with persisted short-term recall entries whose snippets are## Tagesnotizen ## Entscheidungen,## Notes:,- ## Entscheidungen, and- ## 修复备份轮换, then invokes production ranking and apply paths with thresholds set to zero.Evidence after fix: Copied terminal output from that real temp-workspace run:
Observed result after fix: Heading-only scaffolding, trailing-colon scaffold headings, and normalized marker-plus-heading placeholders ranked zero candidates, applied zero promotions, and did not create
MEMORY.md. The durable localized non-colon inline heading bullet- ## 修复备份轮换ranked, applied once, createdMEMORY.md, and appeared in the resulting memory file.What was not tested: I did not run a full live dreaming cron cycle against a private real workspace; the user-reported failure shape is covered with filesystem-backed promotion fixtures and the standalone temp-workspace OpenClaw promotion run above rather than private memory contents.