Skip to content

fix(memory): complete recursive memory search follow-up#72748

Closed
leafbird wants to merge 1 commit into
openclaw:mainfrom
leafbird:fix/34400-memory-recursive-followup-main
Closed

fix(memory): complete recursive memory search follow-up#72748
leafbird wants to merge 1 commit into
openclaw:mainfrom
leafbird:fix/34400-memory-recursive-followup-main

Conversation

@leafbird

Copy link
Copy Markdown

PR Draft — memory recursive search follow-up

Suggested title

fix(memory): complete recursive memory search follow-up

Suggested base/head

  • base: upstream/main
  • head: fix/34400-memory-recursive-followup-main

Summary

This follow-up closes the remaining gaps around issue #34400 for recursive memory search support.

It adds support for nested dated memory paths in temporal decay, ensures dirty search state is synchronized before querying, fixes QMD watcher handling to avoid root-ignore regressions while still filtering non-matching events, and aligns user-facing text with the actual recursive memory/**/*.md behavior.

What changed

  • Support nested dated memory files in temporal decay
    • memory/YYYY-MM/YYYY-MM-DD.md and similar nested paths are now recognized as dated memory files
    • non-ASCII and dotted intermediate directories are covered
  • Ensure dirty search state is synchronized before search
    • search now awaits sync when indexes are dirty instead of fire-and-forget background sync
    • includes a safe closed-manager bail-out after sync
  • Fix QMD watcher behavior
    • watch collection roots instead of relying on path+glob watch targets
    • keep chokidar-level ignore focused on known ignored directories only
    • move collection pattern filtering to the event-handling stage
    • avoid root-ignore startup regressions while still preventing false syncs from non-matching files
  • Align user-facing descriptions
    • update prompt/tool/schema help text from memory/*.md to memory/**/*.md

Validation

Unit / targeted tests

corepack pnpm vitest run \
  extensions/memory-core/src/memory/temporal-decay.test.ts \
  extensions/memory-core/src/memory/manager.async-search.test.ts \
  extensions/memory-core/src/memory/manager.watcher-config.test.ts \
  extensions/memory-core/src/memory/qmd-manager.test.ts
  • Result: 4 files / 107 tests passed

Build

corepack pnpm build
  • Result: success

Live smoke

  • Verified recursive nested memory pickup and first-search freshness in a live smoke run

Real chokidar smoke

  • Verified with a standalone live chokidar smoke that:
    • watched root is not ignored during startup
    • .txt events are ignored at handling stage
    • .md and nested .md events are handled as intended

Live QMD backend end-to-end smoke

  • Verified that:
    • QMD backend actually activated (provider: "qmd", backend: "qmd", effectiveMode: "search")
    • a newly created nested .md file was found on the first search
    • a subsequent change to that file was also reflected in search results
    • a sibling .txt file in the same nested path was not surfaced in search

Limitations / notes

  • Watching collection roots may increase event volume in large workspaces; current assessment is that this is monitor-worthy but not a blocker

Related issue

@greptile-apps

greptile-apps Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR closes remaining gaps from issue #34400 across four areas: nested dated-path support in temporal decay, blocking search-sync to prevent stale first-search results, a QMD watcher refactor that watches collection roots and filters events at the handler stage, and user-facing text alignment to memory/**/*.md.

The core logic changes are well-reasoned and the test coverage (107 tests) is thorough. The only P2 note is that shouldIgnoreMemoryWatchPath accepts a stats parameter that it never reads; marking it _stats would clarify intent.

Confidence Score: 4/5

Safe to merge; only P2 style finding on an unused parameter.

No P0 or P1 issues found. One P2 style observation (unused stats parameter). All four areas of change are logically sound and well tested.

No files require special attention beyond the unused stats parameter in qmd-manager.ts.

Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/memory/qmd-manager.ts
Line: 183-192

Comment:
**Unused `stats` parameter**

`shouldIgnoreMemoryWatchPath` accepts `stats` to match chokidar's callback signature, but the implementation never inspects it. The function currently only needs the path to check for ignored directory name segments. If the intent is purely to satisfy the type signature, a leading underscore (`_stats`) would make this intent explicit and suppress lint warnings.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix: complete memory recursive search fo..." | Re-trigger Greptile

Comment on lines +183 to 192
function shouldIgnoreMemoryWatchPath(
watchPath: string,
stats: { isFile(): boolean; isDirectory(): boolean } | undefined,
): boolean {
const normalized = path.normalize(watchPath);
const parts = normalized
.split(path.sep)
.map((segment) => normalizeLowercaseStringOrEmpty(segment));
return parts.some((segment) => IGNORED_MEMORY_WATCH_DIR_NAMES.has(segment));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unused stats parameter

shouldIgnoreMemoryWatchPath accepts stats to match chokidar's callback signature, but the implementation never inspects it. The function currently only needs the path to check for ignored directory name segments. If the intent is purely to satisfy the type signature, a leading underscore (_stats) would make this intent explicit and suppress lint warnings.

Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/memory/qmd-manager.ts
Line: 183-192

Comment:
**Unused `stats` parameter**

`shouldIgnoreMemoryWatchPath` accepts `stats` to match chokidar's callback signature, but the implementation never inspects it. The function currently only needs the path to check for ignored directory name segments. If the intent is purely to satisfy the type signature, a leading underscore (`_stats`) would make this intent explicit and suppress lint warnings.

How can I resolve this? If you propose a fix, please make it concise.

@leafbird

Copy link
Copy Markdown
Author

Thanks — addressed.

I renamed the unused stats parameter to _stats in shouldIgnoreMemoryWatchPath to make the intent explicit, and re-ran the targeted QMD watcher test:

  • corepack pnpm vitest run extensions/memory-core/src/memory/qmd-manager.test.ts

Result: 93 passed

@clawsweeper

clawsweeper Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge.

Latest ClawSweeper review: 2026-05-20 23:08 UTC / May 20, 2026, 7:08 PM ET.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The PR updates memory-core dirty search sync, nested dated temporal decay, QMD watcher filtering, recursive memory wording, generated config hashes, package metadata, lockfile, and changelog.

Reproducibility: yes. source-reproducible. Current main fire-and-forgets dirty search sync, only matches flat dated memory paths for temporal decay, and passes glob-shaped QMD watch targets despite chokidar v5 documentation saying glob support was removed.

PR rating
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Summary: The implementation has useful signal, but missing real behavior proof plus a blocking descriptor gap and merge conflicts keep it from being quality-ready.

Rank-up moves:

  • Add redacted after-fix proof from a real QMD or builtin memory setup, such as terminal output or logs showing nested-file first search and non-matching file exclusion.
  • Mirror recursive wording in the lazy tool descriptors and public memory docs.
  • Rebase onto current main, resolve conflicts, and rerun focused memory-core validation.
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
Needs real behavior proof before merge: The PR body describes live smokes but provides no inspectable after-fix terminal output, logs, screenshot, recording, or linked artifact; the contributor should add redacted proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • No inspectable after-fix real behavior proof is attached for the claimed live, chokidar, or QMD smoke runs.
  • The PR head is currently conflicting with main and still has failed checks on the exact head, so it cannot be merged as-is.
  • Lazy tool descriptors and public docs can continue telling agents/users to use flat memory/*.md even after the runtime changes land.
  • The related temporal-decay PR covers timestamp-suffixed dated files that this branch does not cover; maintainers should decide whether to fold that coverage here or leave the narrower PR open.

Maintainer options:

  1. Complete and prove this branch (recommended)
    Fix the descriptor/docs gaps, coordinate the temporal-decay overlap, rebase onto current main, and add redacted real QMD or builtin memory proof before merge.
  2. Accept a narrower temporal scope
    Maintainers can intentionally land only exact YYYY-MM-DD nested decay here, but should keep the timestamp-suffix follow-up linked and open.
  3. Pause until contributor proof arrives
    If the contributor cannot provide real behavior proof for their setup, keep the PR paused rather than replacing it with an automated repair branch.

Next step before merge
Contributor-supplied proof, rebase/conflict resolution, and maintainer coordination on the temporal-decay overlap are needed before an automated repair lane would be useful.

Security
Cleared: No concrete security or supply-chain regression found; the new minimatch dependency is exact-pinned and already governed by the root override, and its use is limited to local configured QMD collection pattern matching.

Review findings

  • [P2] Update the lazy tool descriptors — extensions/memory-core/src/tools.ts:240
  • [P3] Update the public memory docs — src/config/schema.help.ts:1057
Review details

Best possible solution:

Land a rebased memory-core follow-up that keeps the runtime fixes, mirrors recursive wording across lazy descriptors and public docs, preserves or explicitly defers timestamp-suffix decay coverage, and includes redacted real behavior proof.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible. Current main fire-and-forgets dirty search sync, only matches flat dated memory paths for temporal decay, and passes glob-shaped QMD watch targets despite chokidar v5 documentation saying glob support was removed.

Is this the best way to solve the issue?

No, not yet. Awaiting dirty sync and moving QMD filtering to the handler are maintainable, but the PR still needs descriptor/docs alignment, rebase, real proof, and a decision on the temporal-decay overlap.

Label changes:

  • add P2: This is a normal memory-core bug-fix PR with limited blast radius but real search/session-state impact.
  • add merge-risk: 🚨 session-state: The PR changes memory indexing freshness, temporal ranking, and QMD watch invalidation paths that determine which memory state an agent sees.
  • add merge-risk: 🚨 availability: The QMD watcher moves to collection-root watching and the PR body acknowledges increased event volume in large workspaces.

Label justifications:

  • P2: This is a normal memory-core bug-fix PR with limited blast radius but real search/session-state impact.
  • merge-risk: 🚨 session-state: The PR changes memory indexing freshness, temporal ranking, and QMD watch invalidation paths that determine which memory state an agent sees.
  • merge-risk: 🚨 availability: The QMD watcher moves to collection-root watching and the PR body acknowledges increased event volume in large workspaces.
  • rating: 🧂 unranked krab: Current PR rating is 🧂 unranked krab because proof is 🧂 unranked krab, patch quality is 🦐 gold shrimp, and The implementation has useful signal, but missing real behavior proof plus a blocking descriptor gap and merge conflicts keep it from being quality-ready.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body describes live smokes but provides no inspectable after-fix terminal output, logs, screenshot, recording, or linked artifact; the contributor should add redacted proof. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Full review comments:

  • [P2] Update the lazy tool descriptors — extensions/memory-core/src/tools.ts:240
    The branch updates the concrete tool descriptions to memory/**/*.md, but the plugin entry still returns lazy memory_search/memory_get descriptors from extensions/memory-core/index.ts before loading that module, and those descriptors still say memory/*.md. Agents can still see the old non-recursive guidance after this change, so mirror the wording in the lazy descriptors too.
    Confidence: 0.89
  • [P3] Update the public memory docs — src/config/schema.help.ts:1057
    The PR updates generated config help, but public docs still describe indexing and on-demand memory as memory/*.md. Users reading the memory docs would still be told the old flat layout, so update the affected docs pages to match the recursive behavior.
    Confidence: 0.84

Overall correctness: patch is incorrect
Overall confidence: 0.84

What I checked:

  • Current main source-repro for stale search and flat dated decay: Current main still calls fire-and-forget dirty search sync in MemoryIndexManager.search, uses a flat dated-memory regex, and watches QMD path-plus-pattern targets; these match the bugs the PR is trying to address. (extensions/memory-core/src/memory/manager.ts:335, b58572e283bc)
  • PR diff implements the core runtime direction: The PR awaits dirty search sync, changes QMD watching to collection roots with handler-stage minimatch filtering, adds awaitWriteFinish, and broadens the dated-memory regex to nested directories. (extensions/memory-core/src/memory/qmd-manager.ts:1571, b871c4a7f869)
  • Dependency contract supports moving filtering out of chokidar globs: Chokidar v5 documentation says v4 removed glob support, documents watching file/dir paths recursively, and supports ignored callbacks plus awaitWriteFinish; this supports the PR's root-watch/filter approach.
  • Lazy descriptors remain stale on main and are not touched by the PR: The plugin entry still exposes lazy memory_search and memory_get descriptors that say memory/*.md; the PR updates extensions/memory-core/src/tools.ts but not extensions/memory-core/index.ts, so agents can still receive stale guidance before the lazy module loads. (extensions/memory-core/index.ts:128, b58572e283bc)
  • Public docs still contain flat memory wording: Relevant public docs still describe memory files as memory/*.md or dated files in memory/ after the branch only updates generated config help and source/tool text. Public docs: docs/concepts/memory-builtin.md. (docs/concepts/memory-builtin.md:78, b58572e283bc)
  • PR proof and mergeability state: Live PR metadata reports mergeable=CONFLICTING/mergeStateStatus=DIRTY, failed real-behavior-proof checks, and no attached inspectable proof artifact beyond PR-body claims. (b871c4a7f869)

Likely related people:

  • steipete: Recent memory-core history includes lazy memory-core runtime surfaces, memory helper export trimming, and broad memory/runtime refactors touching the same plugin entry and manager area. (role: recent area contributor; confidence: high; commits: 2dd3e40a1378, f3d2ae895a26, 694ca50e9775; files: extensions/memory-core/index.ts, extensions/memory-core/src/memory/manager.ts, extensions/memory-core/src/memory/qmd-manager.ts)
  • frankekn: Recent history on qmd-manager includes the watcher fd-pressure fix, which is adjacent to this PR's root-watch and awaitWriteFinish changes. (role: recent watcher contributor; confidence: medium; commits: b04e42812eca; files: extensions/memory-core/src/memory/qmd-manager.ts)
  • giodl73-repo: Recent memory-core commits touch QMD lexical search and stale session catch-up, both adjacent to this PR's search freshness and QMD behavior. (role: recent memory search contributor; confidence: medium; commits: 44c3d8ea2efd, 8af2af24a53c; files: extensions/memory-core/src/memory/qmd-manager.ts, extensions/memory-core/src/memory/manager.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against b58572e283bc.

@leafbird
leafbird force-pushed the fix/34400-memory-recursive-followup-main branch from 3575931 to 77f9f16 Compare May 2, 2026 01:43
@openclaw-barnacle openclaw-barnacle Bot added the docs Improvements or additions to documentation label May 2, 2026
@leafbird
leafbird force-pushed the fix/34400-memory-recursive-followup-main branch from 77f9f16 to 2f0d481 Compare May 2, 2026 01:50
@leafbird
leafbird force-pushed the fix/34400-memory-recursive-followup-main branch 2 times, most recently from 90dd9f3 to 4df486a Compare May 2, 2026 06:42
@leafbird

leafbird commented May 2, 2026

Copy link
Copy Markdown
Author

Heads up — the 4 remaining red checks on this PR look unrelated to the change:

  • check-dependenciesknip flags 8 files under src/tools/* (availability.ts, descriptors.ts, diagnostics.ts, execution.ts, index.ts, planner.ts, protocol.ts, types.ts) as unused. This PR only modifies extensions/memory-core/src/tools.ts; it doesn't touch src/tools/**.
  • checks-fast-contracts-plugins(-c)extension-runtime-dependencies.contract.test.ts fails with extensions/qa-lab does not keep unused direct runtime dependencies — expected [ '@openclaw/qa-channel' ] to deeply equal []. This PR doesn't touch extensions/qa-lab or its dependency manifest.
  • check — rollup of the two above.

After rebasing onto current main, the previous 5 checks-node-* failures all turned green, so the rebase did its job; these 4 look like pre-existing regressions on main. Flagging here in case it's helpful for the review.

@leafbird
leafbird force-pushed the fix/34400-memory-recursive-followup-main branch from 4df486a to 446352d Compare May 3, 2026 14:15
@leafbird

leafbird commented May 3, 2026

Copy link
Copy Markdown
Author

Rebased onto latest main and regenerated docs/.generated/config-baseline.sha256 to resolve the conflict. No code changes; ready for re-review.

@leafbird
leafbird force-pushed the fix/34400-memory-recursive-followup-main branch from 446352d to b871c4a Compare May 10, 2026 15:44
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 10, 2026
@leafbird

Copy link
Copy Markdown
Author

Rebased onto latest upstream/main to resolve conflicts.

Conflicts resolved

  • extensions/memory-core/package.json — kept upstream's json5 and [email protected], plus our [email protected].
  • pnpm-lock.yaml — regenerated via pnpm install after the merged dependency set.
  • src/config/schema.base.generated.ts — accepted upstream's deletion (replaced by runtime schema computation in 55d1cf87d7).
  • docs/.generated/config-baseline.sha256 — regenerated via pnpm config:docs:gen to reflect the new help text.

Local verification

  • corepack pnpm vitest run for the four targeted memory-core test files: 4 files / 123 tests passed
  • corepack pnpm build: success
  • corepack pnpm tsgo:prod: clean
  • corepack pnpm check:no-conflict-markers, check:import-cycles, check:test-types, config:docs:check: all clean

Pre-existing lint note (not introduced by this PR)
Scoped oxlint on the changed files surfaces one no-underscore-dangle error at extensions/memory-core/src/memory/qmd-manager.ts:152 for _hasHanScript. I confirmed this exists on upstream/main itself (introduced by 99a2f5379e and renamed in af62a2c2e4) and is unrelated to the recursive memory search work in this PR. Left as-is so this PR doesn't drag in unrelated cleanup.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 20, 2026
@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels May 22, 2026
@leafbird

Copy link
Copy Markdown
Author

Closing in favor of #85899.

The runtime now walks memory/ recursively (collectMemoryFilesFromDir in packages/memory-host-sdk/src/host/internal.ts), and the QMD watcher fd-pressure work was independently resolved by #81802 (watch-settle.ts). The remaining value of this PR was just the stale memory/*.mdmemory/**/*.md wording alignment, which I've split out as a small behavior-neutral PR #85899 for easier review. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to documentation extensions: memory-core Extension: memory-core merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant