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 5 commits into
Conversation
![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
…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
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
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
…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
|
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 測試路徑問題也看一下嗎？ |
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