fix: Upgrade/compactor text and metadata enrichment + rerank topN cap#789
fix: Upgrade/compactor text and metadata enrichment + rerank topN cap#789jlin53882 wants to merge 11 commits into
Conversation
…68/PR708 clean rebuild) Rebuild of PRs CortexReach#668 and CortexReach#708 with cleaner scope. Changes: - Add signal?: AbortSignal to RetrievalContext interface - Add signal param to vectorOnlyRetrieval + hybridRetrieval - Pass signal to embedQuery(query, signal) for true cancellation - Add try/catch in embedQuery: catch → BM25 fallback (James's design) - Remove useLightweightAutoRecall (replaced by fallback design) - Fix retrieveWithTrace() missing mode/diagnostics declarations Design: normal path uses vector/hybrid; if embed fails (timeout/network/API error), fallback to BM25 instead of returning nothing. Doesn't change default behavior. Tests: - retriever-auto-recall-signal.test.mjs: signal propagation + BM25 fallback Refs: PR CortexReach#668, PR CortexReach#708, PR CortexReach#746
![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: c13198795e
ℹ️ 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 (seenTexts.has(e.text)) { | ||
| skipped++; | ||
| skippedDedup++; | ||
| continue; | ||
| } | ||
| seenTexts.add(e.text); |
There was a problem hiding this comment.
When importing multiple workspaces at once, this in-memory dedup only keys on e.text, so the second workspace's identical bullet is skipped before the later scope-aware retrieve({ scopeFilter: [e.effectiveScope] }) check can run. That breaks the existing per-scope import behavior and silently drops memories that should be stored separately in different scopes; the key needs to include effectiveScope as well as the text.
Useful? React with 👍 / 👎.
| } else { | ||
| console.log(`[import] embedded batch ${batchIdx}/${totalBatches} (${batch.length} entries)`); | ||
| await flushPending(); |
There was a problem hiding this comment.
With the default batchSize of 10 and more than one embed batch, this else branch immediately calls flushPending() even though pendingFlush.length is still below FLUSH_THRESHOLD, so imports flush every embed batch instead of batching up to 100 entries. For large markdown imports this turns the intended 100-entry bulk writes back into many small bulkStore calls and lock acquisitions, undermining the performance fix.
Useful? React with 👍 / 👎.
jlin53882
force-pushed
the
fix/upgrader-compactor-text-enrichment
branch
from
c131987 to
d2c4471
Compare
When embedQuery() throws due to an abort signal, we now re-throw immediately instead of wasting resources doing BM25 fallback retrieval. Only BM25 fallback is attempted for non-abort errors (network/API failures). Fixes PR784 review comment by TurboTheTurtle: "aborted embedding requests still continue into BM25 fallback work" Updated tests: - AbortError re-thrown in vectorOnlyRetrieval (signal already aborted) - AbortError re-thrown in hybridRetrieval (signal already aborted) - BM25 fallback used for non-abort errors (network error, signal not aborted)
…o buildRerankRequest This ensures ALL rerank providers are limited to candidatePoolSize, not just those that support topN in their API body (jina/pinecone/voyage). tei and dashscope ignore the topN parameter, so previously they would receive all candidates regardless of candidatePoolSize setting. Now documents are sliced BEFORE mapping, ensuring tei/dashscope also receive limited candidates (even though they ignore topN). Issue: CortexReach#789 (follow-up)
tei/dashscope Provider Note (Additional Fix)Important: tei and dashscope providers do not support the opN parameter in their API body. Originally, the opN cap would only work for jina/pinecone/voyage providers. Fix applied: documents are now sliced to candidatePoolSize BEFORE passing to �uildRerankRequest: \\ ypescript // After: Only candidatePoolSize results are mapped This ensures ALL rerank providers (including tei/dashscope) receive limited candidates, even though tei/dashscope ignore the opN parameter in their API body. Additional fix by: Claude Code |
tei/dashscope Provider Note (Additional Fix)Important: tei and dashscope providers do NOT support the topN parameter in their API body. Originally, the topN cap would only work for jina/pinecone/voyage providers. Fix applied: In retriever.ts, documents are now sliced to candidatePoolSize BEFORE passing to buildRerankRequest: ` // After: Only candidatePoolSize results are mapped This ensures ALL rerank providers (including tei/dashscope) receive limited candidates, even though tei/dashscope ignore the topN parameter in their API body. Additional fix by: Claude Code |
jlin53882
force-pushed
the
fix/upgrader-compactor-text-enrichment
branch
from
4cc9a84 to
2e22fe9
Compare
|
Review finding: P2: new regression test is not wired into CI This PR adds |
jlin53882
force-pushed
the
fix/upgrader-compactor-text-enrichment
branch
from
576600f to
2e22fe9
Compare
…o buildRerankRequest This ensures ALL rerank providers are limited to candidatePoolSize, not just those that support topN in their API body (jina/pinecone/voyage). tei and dashscope ignore the topN parameter, so previously they would receive all candidates regardless of candidatePoolSize setting. Now documents are sliced BEFORE mapping, ensuring tei/dashscope also receive limited candidates (even though they ignore topN). Issue: CortexReach#789 (follow-up)
jlin53882
force-pushed
the
fix/upgrader-compactor-text-enrichment
branch
from
507f31b to
c5b1c51
Compare
測試結果更新 (2026-05-12)實際測試結果\ Suites:
變更的檔案 (5 files)
。 |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
PR #789 Review: fix: Upgrade/compactor text and metadata enrichment + rerank topN cap
Verdict: REQUEST-CHANGES | 6 rounds completed | Value: 74% | Size: LARGE | Author: jlin53882
Value Assessment
Problem: The PR addresses a retrieval-quality regression where upgraded and compacted memories can lose or omit the L0/L1/L2 smart metadata structure, reducing BM25/FTS recall. It also caps rerank candidate volume so provider calls are not overwhelmed by large result pools.
| Dimension | Assessment |
|---|---|
| Value Score | 74% |
| Value Verdict | priority-review |
| Issue Linked | true |
| Project Aligned | true |
| Duplicate | false |
| AI Slop Score | 0/6 |
| User Impact | high |
| Urgency | high |
Open Questions:
- Confirm whether issue #786 has maintainer acceptance outside the provided context, since labels, assignment, and maintainer issue comments are not shown.
- Deep review should verify the new compactor metadata defaults, especially tier, confidence, and category behavior, match project expectations.
Summary
The PR addresses a retrieval-quality regression where upgraded and compacted memories can lose or omit the L0/L1/L2 smart metadata structure, reducing BM25/FTS recall. It also caps rerank candidate volume so provider calls are not overwhelmed by large result pools.
Evaluation Signals
| Signal | Value |
|---|---|
| Blockers | 0 |
| Warnings | 0 |
| PR Size | LARGE |
| Verdict Floor | approve |
| Risk Level | high |
| Value Model | codex |
| Primary Model | codex |
| Adversarial Model | claude |
Must Fix
- F1: Compactor writes legacy categories into smart metadata
Nice to Have
- F2: Merged entries inherit lifecycle state from one source entry
- F3: Rerank cap test passes through fallback path
- MR1: Compaction resets access_count to 0, discarding retrieval-frequency signal from source entries
- MR2: Compactor unconditionally sets tier to "working", overriding any source-entry tier promotion
- MR3: l0_abstract is a raw first-line truncation, not a semantically meaningful abstract — degrades the very BM25/FTS path the PR claims to fix
Recommended Action
Good direction — problem is worth solving. Author should address must-fix findings, then this is ready to merge.
Reviewed at 2026-05-12T17:03:24Z | 6 rounds | Value: codex | Primary: codex | Adversarial: claude
|
F1 修復完成 - Compactor 現在使用 reverseMapLegacyCategory 將 legacy category 轉換為 memory_category 格式。其他可選內容 (F2/F3/MR1/MR2/MR3) 分析後建議維持現狀。PR789 準備好可以 merge. |
PR789 Review 回覆：F1 修復 + 可選內容分析F1 Must Fix：Compactor writes legacy categories into smart metadata問題： 修復： // import 反向轉換函式
import { reverseMapLegacyCategory } from "./smart-metadata.js";
// 在 buildMergedEntry 中：
- memory_category: category,
+ const smartCategory = reverseMapLegacyCategory(legacyCategory, text);
+ memory_category: smartCategory,Commit: 其他可選內容分析
結論：F1 已修復，其他項目建議維持現狀。 PR789 現況
需要我把 F3 測試路徑問題也看一下嗎？ |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
PR #789 Review: fix: Upgrade/compactor text and metadata enrichment + rerank topN cap
Verdict: REQUEST-CHANGES | 6 rounds completed | Value: 77% | Size: LARGE | Author: jlin53882
Value Assessment
Problem: Upgraded and compacted memories can lose or omit the full L0/L1/L2 smart metadata structure, causing BM25/FTS retrieval quality regressions. The PR also limits rerank candidate volume to avoid sending excessive documents to rerank providers.
| Dimension | Assessment |
|---|---|
| Value Score | 77% |
| Value Verdict | priority-review |
| Issue Linked | true |
| Project Aligned | true |
| Duplicate | false |
| AI Slop Score | 0/6 |
| User Impact | high |
| Urgency | high |
Open Questions:
- Confirm whether issue #786 is maintainer-accepted; the supplied issue metadata has no labels, assignment, or maintainer issue comments.
- Deep review should verify the compactor defaults for tier, access_count, confidence, and l0_abstract generation match intended lifecycle and ranking semantics.
- Because stale_base is true, confirm the branch is rebased and current CI still passes before merge.
Summary
Upgraded and compacted memories can lose or omit the full L0/L1/L2 smart metadata structure, causing BM25/FTS retrieval quality regressions. The PR also limits rerank candidate volume to avoid sending excessive documents to rerank providers.
Evaluation Signals
| Signal | Value |
|---|---|
| Blockers | 0 |
| Warnings | 0 |
| PR Size | LARGE |
| Verdict Floor | approve |
| Risk Level | high |
| Value Model | codex |
| Primary Model | codex |
| Adversarial Model | claude |
Must Fix
- F1: Compactor still drops smart metadata L2 content
Nice to Have
- F2: Merged metadata inherits lifecycle flags from first source
- F3: Rerank regression test passes through fallback path
- MR1: Upgrader test does not exercise the changed code path
- MR2: access_count reset to 0 on compaction destroys historical access signal
- MR3: tier hardcoded to 'working' regardless of source tiers
- MR4: l0_abstract truncation at 120 bytes can split multi-byte UTF-8 characters
- MR5: l1_overview only samples first 3 lines, biased toward members[0]
Recommended Action
Good direction — problem is worth solving. Author should address must-fix findings, then this is ready to merge.
Reviewed at 2026-05-14T08:23:19Z | 6 rounds | Value: codex | Primary: codex | Adversarial: claude
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
PR #789 Review: fix: Upgrade/compactor text and metadata enrichment + rerank topN cap
Verdict: REQUEST-CHANGES | 6 rounds completed | Value: 73% | Size: LARGE | Author: jlin53882
Value Assessment
Problem: Upgraded and compacted memories can lose or omit the L0/L1/L2 smart metadata structure, reducing BM25/FTS retrieval recall. The PR also limits rerank candidate volume so providers are not sent excessive documents.
| Dimension | Assessment |
|---|---|
| Value Score | 73% |
| Value Verdict | priority-review |
| Issue Linked | true |
| Project Aligned | true |
| Duplicate | false |
| AI Slop Score | 2/6 |
| User Impact | high |
| Urgency | high |
Scope Drift: 1 flag(s)
- .claude/adversarial-review-prompt.txt is not justified by PR #789 and references PR #745, Branch: james/issue-692-ast-chunking, and HEAD: 99da011
AI Slop Signals:
- .claude/adversarial-review-prompt.txt is unrelated to the stated PR #789 fix and discusses PR #745 instead
- test/upgrader-compactor-rerank-fixes.test.mjs claims upgrader l2_content coverage, but the shown upgrader test exercises the null-LLM fallback path rather than a successful l2_content enrichment path
Open Questions:
- Issue #786 has no labels, assignment, or maintainer issue comments in the supplied context; confirm maintainer acceptance.
- stale_base is true; confirm the branch is rebased and current CI still passes.
- Confirm whether .claude/adversarial-review-prompt.txt should be removed because it references PR #745, not PR #789.
- Deep review should verify compactor semantics for l0_abstract truncation, tier/access_count inheritance, category mapping, and l2_content preservation.
Summary
Upgraded and compacted memories can lose or omit the L0/L1/L2 smart metadata structure, reducing BM25/FTS retrieval recall. The PR also limits rerank candidate volume so providers are not sent excessive documents.
Evaluation Signals
| Signal | Value |
|---|---|
| Blockers | 0 |
| Warnings | 0 |
| PR Size | LARGE |
| Verdict Floor | approve |
| Risk Level | high |
| Value Model | codex |
| Primary Model | codex |
| Adversarial Model | claude |
Must Fix
- F1: Compacted entries inherit stale lifecycle metadata from the first source
Nice to Have
- F2: UTF-8 byte truncation can insert replacement characters
- F3: Rerank cap regression test does not cover the cap
- MR1: Unrelated developer artifact .claude/adversarial-review-prompt.txt committed in this PR
- MR3: Tier and access_count inherited only from members[0] regardless of other members' state
- MR4: No regression test guards the F1 lifecycle-metadata inheritance case
Recommended Action
Good direction — problem is worth solving. Author should address must-fix findings, then this is ready to merge.
Reviewed at 2026-05-14T09:35:23Z | 6 rounds | Value: codex | Primary: codex | Adversarial: claude
…o buildRerankRequest This ensures ALL rerank providers are limited to candidatePoolSize, not just those that support topN in their API body (jina/pinecone/voyage). tei and dashscope ignore the topN parameter, so previously they would receive all candidates regardless of candidatePoolSize setting. Now documents are sliced BEFORE mapping, ensuring tei/dashscope also receive limited candidates (even though they ignore topN). Issue: CortexReach#789 (follow-up)
…ew fairness + tests All 6 Nice-to-Have items from PR CortexReach#789 review: - F2/MR3: tier inherited from highest-priority source (core > working > peripheral) - MR2: access_count inherited as average of source members (not reset to 0) - MR4: l0_abstract truncation uses safeTruncate() to avoid splitting UTF-8 chars - MR5: l1_overview samples across all members fairly (not just members[0]) - MR1: new upgrader test covers l2_content success path (LLM-enriched entry) - F3: rerank candidatePoolSize bounded in DEFAULT_RETRIEVAL_CONFIG (smoke test) Also install jiti devDependency (was missing, blocking test runs) and bump to latest jiti v2.7.0. F1 (buildSmartMetadata call in buildMergedEntry) was already present. Existing 23 compactor tests + 11 fix tests all pass. Refs: CortexReach#786
jlin53882
force-pushed
the
fix/upgrader-compactor-text-enrichment
branch
from
33eeccf to
fda20fd
Compare
|
Thanks for the follow-up fixes. This branch is still conflicting with current Please rebase onto latest After resolving, please run: npm run test:packaging-and-workflow
npm run test:core-regressionPing when the branch is conflict-free and CI is green so I can re-review the updated diff. |
Issue CortexReach#786 findings addressed: - upgrader: text now uses l2_content (not l0_abstract) for BM25/FTS - compactor: buildMergedEntry produces proper L0/L1/L2 metadata - retriever: rerank topN capped at candidatePoolSize PR: CortexReach#789
…o buildRerankRequest This ensures ALL rerank providers are limited to candidatePoolSize, not just those that support topN in their API body (jina/pinecone/voyage). tei and dashscope ignore the topN parameter, so previously they would receive all candidates regardless of candidatePoolSize setting. Now documents are sliced BEFORE mapping, ensuring tei/dashscope also receive limited candidates (even though they ignore topN). Issue: CortexReach#789 (follow-up)
…rankRequest - Document tei/dashscope/pinecone/voyage/jina encoding differences - Note that documents are sliced before this function for all providers
…ession group Fixes maintainer review finding: GitHub Actions runs ci-test-manifest.mjs, not npm test, so the test was never exercised in PR CI. Issue: CortexReach#786 PR: CortexReach#789
…eMapLegacyCategory F1 fix: Compactor now converts legacy category to new memory_category format using reverseMapLegacyCategory() before writing to smart metadata. This ensures: - Legacy categories (preference/Fact/...) are mapped to new format (preferences/facts/entities/...) - Smart metadata structure is consistent - scoreLexicalHit reads correct memory_category without conflict Issue: CortexReach#786 Fixes: PR#789 review F1 (CHANGES_REQUESTED) PR: CortexReach#789
…ew fairness + tests All 6 Nice-to-Have items from PR CortexReach#789 review: - F2/MR3: tier inherited from highest-priority source (core > working > peripheral) - MR2: access_count inherited as average of source members (not reset to 0) - MR4: l0_abstract truncation uses safeTruncate() to avoid splitting UTF-8 chars - MR5: l1_overview samples across all members fairly (not just members[0]) - MR1: new upgrader test covers l2_content success path (LLM-enriched entry) - F3: rerank candidatePoolSize bounded in DEFAULT_RETRIEVAL_CONFIG (smoke test) Also install jiti devDependency (was missing, blocking test runs) and bump to latest jiti v2.7.0. F1 (buildSmartMetadata call in buildMergedEntry) was already present. Existing 23 compactor tests + 11 fix tests all pass. Refs: CortexReach#786
jlin53882
force-pushed
the
fix/upgrader-compactor-text-enrichment
branch
from
fda20fd to
813c5ac
Compare
…ugin.json location Fixes Windows path bug where path.resolve(new URL().pathname) produced 'C:\\C:\\...' double drive letter path. Uses existing testDir constant from fileURLToPath(import.meta.url) instead.
CI Update (May 18)Bug Fixed: ier1-counters.test.mjs Windows path bugRoot cause: Test used path.resolve(path.dirname(new URL(import.meta.url).pathname), '..', 'openclaw.plugin.json') which produced C:\C:... double drive letter path on Windows. Fix: Changed to use existing estDir constant (from ileURLToPath(import.meta.url)) which works correctly on Windows. Commit: 2e5d321 on ix/upgrader-compactor-text-enrichment branch CI StatusAll 6 CI jobs now pass:
Branch is rebased and conflict-free. Ready for maintainer review. |
CI 分析結果已完成的修復
本地測試驗證upgrader-compactor-rerank-fixes.test.mjs: 11/11 pass core-regression CI 失敗分析結論：與 PR789 無關，是預存問題。 CI core-regression 在 commit 813c5ac（PR789 最初版本）就已經 fail 懷疑是 GitHub Actions runner 環境問題或某個測試的基礎設施問題。建議後續調查。 PR789 程式碼狀態
分析由 AI 完成，本地測試驗證通過 |
PR #789 Review: fix: Upgrade/compactor text and metadata enrichment + rerank topN capVerdict: RESOLVE-CONFLICTS-FIRST | Author: jlin53882 | Merge state: DIRTY
Problem Statement (R1)Upgraded and compacted memories can lose or omit full L0/L1/L2 smart metadata, reducing BM25/FTS recall quality. The PR also limits rerank input size to avoid sending excessive candidates to rerank providers. Why This Stopped HereGitHub reports
Recommended ActionAuthor should:
Reviewed at 2026-05-23T09:00:59Z | R0+R1 gate | Conflict gate |
3 participants
Summary
Fix for memory-upgrader 和 memory-compactor 破壞 text/l0_abstract 結構 (Issue #786)
Changes
src/memory-upgrader.tsl2_content(notl0_abstract) to preserve full content for BM25/FTSsrc/memory-compactor.tsbuildMergedEntrynow produces proper L0/L1/L2 metadata viabuildSmartMetadatasrc/retriever.tstopNcapped atcandidatePoolSizeto prevent overwhelming rerank APItest/upgrader-compactor-rerank-fixes.test.mjsIssue #786 Findings Addressed
l0_abstract(30 chars), now usesl2_content(full content) for better FTSbuildSmartMetadataresults.length), now capped atcandidatePoolSizeTest Results
Verification
Original issue was that:
text = l0_abstract→ BM25/FTS only indexed ~30 charsundefinedas search textAll three are fixed in this PR.
Generated by: Review Claw (稽核爪)
Date: 2026-05-08