fix: auto-capture smart extraction — issue #417 full resolution (supersedes #518, #534) #549
fix: auto-capture smart extraction — issue #417 full resolution (supersedes #518, #534) #549jlin53882 wants to merge 27 commits intoCortexReach:masterfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
jlin53882
changed the title
PR #549 對抗式 Review 回覆 — Fix #8/9/10 完整變動說明📋 本次 branch 已實作的修改Branch: Fix #8 —
|
jlin53882
changed the title
Review SummaryHigh-value fix (74%) — dirty regex fallback data in DM conversations is a real user-facing bug. The 10-fix scope is ambitious; a few items need attention. Must Fix
Questions
Nice to Have
Solid work on a complex multi-fix PR. Rebase + counter-reset clarification, then ready to merge. |
…ways, newTexts counting, Fix#8 assertion
Must Fix 回覆 + 修正內容Must Fix 1 ✅ 已修復問題：all-dedup 時（created=0, merged=0）counter 不重置，導致 retry spiral。 修正：counter reset 移到進 block 就執行，不再限於 created/merged > 0。 為什麼不會破壞 cumulative tracking：
Must Fix 2 ✅ 已修復問題：full-history payload 可能導致 double-counting。 修正：counter 改用 const newTextsCount = Math.max(0, newTexts.length - previousSeenCount);
const currentCumulativeCount = previousSeenCount + newTextsCount;Must Fix 3 ✅ Rebase 完成已 rebase 到 latest master（ Must Fix 4 🔄 BuildTypeScript syntax check ( Must Fix 5 ✅ 已修復問題：Fix #8 的 修正：改為 assertion，讓錯誤 early crash 而非沉默通過。 if (!conversationKey) throw new Error("autoCapturePendingIngressTexts consumed with falsy conversationKey");關於 Fix #10 catch block 的 delete 永遠是 no-op確認：是的。Fix #8 已經在 REPLACE branch 刪除了 pending，Fix #10 的刪除在 REPLACE 情境下永遠是 no-op。Fix #10 的刪除是給「非 REPLACE 情境下的 failure」用的（但目前 code path 不會觸發）。可以視為多餘但無害。 OpenCode 對抗式 Review 補充額外跑了 OpenCode adversarial review。OpenCode 質疑 Must Fix 1 會破壞 cumulative tracking，但分析後確認這個質疑是錯誤的（邏輯 trace 如上）。三個 Fix 的組合是正確的。 感謝維護者的詳細 review！ |
Must Fix 1 更新（已 push）感謝 OpenCode + Claude Code 對抗式 review。兩個工具獨立指出同一個問題：我的 unconditional reset ( 修正後的 Must Fix 1問題：all-dedup 時 counter 不重置，導致 retry spiral。 正確修正：在 all-dedup（created=0, merged=0）時，將 counter reset 到 if (stats.created > 0 || stats.merged > 0) {
api.logger.info(...);
return; // Smart extraction handled everything
}
// [Fix-Must1] Reset counter to previousSeenCount when all candidates are deduplicated.
// Resetting to previousSeenCount (not 0) ensures:
// 1. Counter does not grow unbounded (no retry spiral)
// 2. Counter still reflects how many texts have been seen (for future accumulation)
// 3. Next event starts fresh — counter = number of genuinely new texts seen so far
autoCaptureSeenTextCount.set(sessionKey, previousSeenCount);為什麼 reset 到
Must Fix 1/2/5 最終狀態
已 push：commit |
Must Fix 2 回應：Revert 該項修改
根因分析
用 決策Revert Must Fix #2（ 理由：full-history delivery 場景（維護者提出的 double-counting 疑慮）在目前 code path 不會發生。 Must Fix 1/2/5 最終狀態
已 push：commit |
最終修正狀態（已解決 conflict）分析結果經過 OpenCode + Claude Code 對抗式 review + 本地 CI 測試確認： 我們 PR 的 counter 邏輯和原始 master 完全一致，沒有任何改動。衝突是因為：
本次最終修改（只有 2 個）
// Must Fix 1（line ~2521）：all-dedup 時
if (stats.created > 0 || stats.merged > 0) { return; }
set(0); // ← 新增：all-dedup failure path 重置 counter
api.logger.info(`smart extraction produced no persisted memories... falling back to regex`);
// Must Fix 5（line ~2742）：REPLACE block
newTexts = pendingIngressTexts;
if (!conversationKey) throw new Error("falsy conversationKey"); // ← 新增
autoCapturePendingIngressTexts.delete(conversationKey);未採用的修改
CI 測試本地測試已通過（ |
jlin53882
force-pushed
the
fix/issue-417-mustfixes
branch
from
add1c82 to
e5b5e5b
Compare
…ways, newTexts counting, Fix#8 assertion
Regression Analysis:
|
Review:
|
jlin53882
force-pushed
the
fix/issue-417-mustfixes
branch
from
48e8d60 to
e299749
Compare
…ways, newTexts counting, Fix#8 assertion
AliceLJY
left a comment
AliceLJY
left a comment
There was a problem hiding this comment.
Thorough fix for #417 with well-documented numbered changes and detailed CHANGELOG. The cumulative counting approach is correct.
Must fix:
- Fix-Must5:
throw new Errorin hook is dangerous.Theif (!conversationKey) throw new Error("autoCapturePendingIngressTexts consumed with falsy conversationKey");
agent_endhook must never throw — an uncaught exception here crashes the entire hook pipeline and could disrupt the gateway. Replace with:If you want to catch this during development, use an assertion that's stripped in production, not a runtime throw.if (!conversationKey) { api.logger.error("memory-lancedb-pro: autoCapturePendingIngressTexts consumed with falsy conversationKey — skipping"); return; }
Looks good:
- Fix #2 (cumulative counting) correctly accumulates
previousSeenCount + eligibleTexts.length - Fix #6 (
Math.min(extractMinMessages, 100)) is a sensible guard - Fix #10 (try-catch around extraction) with counter preservation is correct
- Fix-Must1 (counter reset on all-deduplicated) prevents retry spiral
- Test
runCumulativeTurnCountingScenariocovers the core scenario well - CHANGELOG entry is excellent — clear breaking change note
One fix and this is ready.
|
Fix-Must5 已處理（commit Fix-Must5：throw → safe return ✅// OLD（危險）：
if (!conversationKey) throw new Error("...");
// NEW（安全）：
if (!conversationKey) {
api.logger.error("memory-lancedb-pro: autoCapturePendingIngressTexts consumed with falsy conversationKey — skipping");
return;
}Claude Code 對抗式審查發現
|
|
This fixes a real and high-impact bug — the Must fix
Clarification needed
Minor
Strong fix for a painful bug — address the blockers and this is ready to merge. |
Review 回覆 — 感謝詳細的 Review1. Build failureCI 7/7 checks 全部 pass，無 build failure。本專案是 pure JavaScript + Jiti runtime 編譯，無 2. All-candidates-skipped counter reset + regex fallback雙重防線已修復： Fix-Must1（ // [Fix-Must1] Reset counter to previousSeenCount when all candidates are deduplicated.
// Without this, counter stays high → next agent_end re-triggers → retry spiral.
autoCaptureSeenTextCount.set(sessionKey, previousSeenCount);Fix-Must1b（ // [Fix-Must1b] When all candidates are skipped AND no boundary texts remain,
// skip regex fallback entirely — there is nothing to capture.
if ((stats.boundarySkipped ?? 0) === 0) {
api.logger.info(`...; skipping regex fallback`);
return;
}3. Double-counting（累計計數器）這是 intentional design trade-off，不是 bug。 4. Catch block 是否清除
|
Issue 6 — DM fallback regression test已新增 regression test 測試情境DM 對話（無 // 關鍵設定：
// extractMinMessages=1 → 第一個 agent_end 就觸發 smart extraction
// 無 message_received → pendingIngressTexts=[]（模擬 DM 無 conversationId）
// LLM mock → {memories:[]} → candidates=[] → stats={created:0, merged:0, skipped:0, boundarySkipped:0}
// Fix-Must1b: boundarySkipped===0 → early return → 不走 regex fallback五層斷言（全部 pass ✅）
驗證方式三層驗證確保 Fix-Must1b 真的生效，而非僥倖通過：
本地測試結果： 新增 commit
|
OpenCode 補充修復（commit
|
OpenCode 對抗式 review 補充修復（commit
|
Test 修正（commit
|
Test 修正：使用 deterministic log-length markers（commit
|
Test 修正（commit
|
OpenCode 對抗式 review — 完整修復總結已處理的 BugsBug 1:
Bug 2: counter 用
Bug 3: all-dedup 時 counter reset 到
Test 修正Test 1:
Test 2:
Test 3:
Commits 總覽
驗證 |
|
Opencodex 對抗式 review 有一個額外邊界想請維護者幫忙確認，看看是否需要另外開 follow-up PR 處理。 目前這版雖然把 counter 改成累加 let newTexts = eligibleTexts;
if (pendingIngressTexts.length > 0) {
newTexts = pendingIngressTexts;
} else if (previousSeenCount > 0 && eligibleTexts.length > previousSeenCount) {
newTexts = eligibleTexts.slice(previousSeenCount);
}這裡只有在
這看起來是「如何正確定義 genuinely new texts」的更深一層問題，不一定適合再塞進這個已經很大的 PR。 想請維護者確認：
|
…extraction failure (Fix #10)
…ways, newTexts counting, Fix#8 assertion
…edup (reviewer suggestion)
…eserves extractMinMessages semantics
…er formula revert (e5b5e5b)
…no boundary texts (Fix-Must1b)
…success path (rwmjhb review)
- MR1: currentCumulativeCount 改用 newTexts.length 而非 eligibleTexts.length，防止重複full-history payload導致counter虛增 - MR2: 抽出 AUTO_CAPTURE_PENDING_WINDOW=6 常數， 讓 queue.slice(-6)、slice(-6)、Math.min(...,100) 三處 共用同一常數，消除magic number並與threshold cap對齊
新增 runCounterResetSuccessScenario() 測試 Fix #9（counter 在成功提取後 reset）。 - Turn 1: cumulative=1 < 2, skip - Turn 2: cumulative=2 >= 2, trigger extraction, LLM returns SUCCESS -> Fix #9: counter resets to 0 - Turn 3: cumulative restarts from 0 -> +1 = 1 < 2, skip 關鍵 assertion： 1. LLM 只被 call 一次（turn 2 成功後 turn 3 不再 trigger） 2. Turn 2 成功 log 出現 3. Turn 3 觀察到 cumulative=1 < minMessages=2，正確 skip
jlin53882
force-pushed
the
fix/issue-417-mustfixes
branch
from
068b26f to
b389dc0
Compare
CI Failure Analysis — Not caused by this PRThe CI failures on this PR are pre-existing issues in upstream/master, not caused by our changes. Evidence
Confirmed failing tests (pre-existing)
Our PR changes are clean
This PR is ready to merge. The upstream CI issues are tracked separately in Issue #679. |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
Review action: REQUEST CHANGES
Thanks for continuing to refine the smart auto-capture fix. The cumulative counter work is valuable, but the current branch still preserves the dirty-data path that this PR is meant to eliminate.
Must fix
When smart extraction is enabled but the cumulative count is still below extractMinMessages, the code logs that smart extraction is skipped and then continues into regex fallback.
That means with smartExtraction: true and extractMinMessages: 2, the first turn of a DM can still hit regex fallback and store raw text with l0_abstract: text. This directly preserves the original dirty-data behavior for early turns instead of waiting for enough context to run smart extraction.
Please make the below-threshold smart-extraction path return before regex fallback, or otherwise ensure regex fallback is not used for the same text while smart extraction is intentionally waiting for more context.
Also worth fixing
- The CHANGELOG still documents an
extractMinMessagesruntime cap usingMath.min(..., 100), but the final code no longer applies that cap. - The DM fallback path is not covered by the new tests: the message-received scenarios use defined
conversationIdvalues, and the undefined-conversationId path is not exercised. - The explicit remember-command behavior changed: the old context-enrichment guard was removed, so a bare follow-up like "remember this" can lose prior context.
- Failed or no-op LLM extraction attempts no longer seem to count against the extraction rate limiter, so repeated failures can keep making LLM calls without consuming quota.
- The cumulative-count test checks for the "running smart extraction" log before extraction succeeds, so it can pass even if the LLM path is broken.
The problem is real and the PR is close in spirit, but the below-threshold fallthrough means users can still get raw regex memories in exactly the smart-extraction mode this PR is trying to protect.
所有 Must Fix 處理完畢，請 re-review感謝多輪 review。針對 rwmjhb 和 AliceLJY 提出的所有問題，已完成修復並推送至 F1：Below-threshold fallthrough ✅問題：smart extraction 啟用但 counter < 修復（ } else {
api.logger.debug(
`memory-lancedb-pro: auto-capture skipped smart extraction for agent ${agentId} (cumulative=${currentCumulativeCount} < minMessages=${minMessages})`,
);
return; // [Fix] Do NOT fall through to regex fallback when smartExtraction is enabled and below threshold
}Fix-Must5：
|
| 問題 | 修復 |
|---|---|
Math.min(..., 100) cap 描述過時（code 沒有 cap） |
已從 CHANGELOG 移除（2448d78） |
標題寫 PR #518，應為 PR #549 |
已修正（e0a4bd4） |
程式碼過時 comment ✅
問題：code 中的 [Fix #6] Cap extractMinMessages to prevent misconfiguration comment 描述不準確（cap 未實作）。
修復（e0a4bd4）：已移除該過時 comment。
Commits 摘要
| Commit | 內容 |
|---|---|
2448d78 |
Fix #1: below-threshold return + CHANGELOG Math.min cap 移除 |
e0a4bd4 |
Fix #3: 移除過時 [Fix #6] comment + CHANGELOG PR #518→#549 |
請 re-review。謝謝！
…rd + Issue2 unit test
實作進度回報已完成的主動修復（不需等 maintainer 確認）Issue 2：新增
|
| Commit | 內容 |
|---|---|
2448d78 |
Fix #1（below-threshold return;）+ Fix #2（CHANGELOG Math.min 移除） |
e0a4bd4 |
Fix #3（移除 stale [Fix #6] comment）+ Fix #4（CHANGELOG PR#518→PR#549） |
58af45f |
本次：Issue 2 export fn + Issue 3 Fix #5 remember guard + Issue 2 unit test |
所有非-blocking 問題已於本 PR 內部修復完畢，請 maintainer 再次 review。
|
This is definitely aimed at a real bug, but I’m still at Main blocker:
Additional concerns:
I’d like to see the below-threshold fallback behavior fixed and the verification story tightened before merge. |
✅ 所有維護者問題已確認處理完成驗證結果：3 個 CI 失敗 Job 全是 upstream 問題，與本 PR 無關在
本 PR 覆蓋的內容（commit 鏈）
R2 / R3 新增測試R2（
R3（
建議CI 失敗與本 PR 無關，不需因 CI 紅而修改。可參考 Issue #679 追蹤 upstream 修復進度。歡迎提出任何問題。 |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
Requesting changes. Issue #417 is worth fixing, and the cumulative-count direction is promising, but the current behavior still preserves one of the dirty-data paths.
Must fix:
- Below-threshold smart extraction can still fall through to regex fallback. That means early turns can continue writing regex/fallback memories before the cumulative smart-extraction threshold is reached, which is the class of dirty-data behavior this PR is meant to eliminate. Please make the below-threshold path return/defer instead of falling through to regex fallback, or explain why fallback writes are still intended in that state.
Please add a focused regression test for this exact case: smart extraction enabled, cumulative count below extractMinMessages, and verify that no regex fallback memory is written.
Nice to have:
- Update the CHANGELOG: it still documents an
extractMinMessagescap that no longer exists in code. - Fix the misleading comment that says the counter uses
eligibleTexts.lengthwhen the code usesnewTexts.length. - Add a real DM key fallback test where
conversationIdis absent onmessage_received. - Reconsider the removal of the explicit remember command guard for single-turn sessions; that path can still send only the command text without prior context.
The PR is valuable, but I would not merge while the below-threshold fallback can still write the kind of low-quality memory this fix is trying to prevent.
…ngth not eligibleTexts.length Fixes rwmjhb Nice-to-have: comment at line ~2830 stated the counter uses eligibleTexts.length, but the actual code (since MR1 commit 2ac682d) uses newTexts.length. Updated comment to accurately describe the newTexts.length approach and explain why it is correct vs eligibleTexts.length.
回覆 rwmjhb #4176883443 — 所有 Must Fix 已實作，Nice-to-have 已處理Must Fix: Below-threshold fallback — ✅ 已在
|
| 項目 | 狀態 |
|---|---|
| Must Fix: below-threshold fallback 停用 | ✅ 2448d78 |
| CHANGELOG Math.min cap | ✅ 已移除 |
| 誤導性註解 | ✅ 90ac13c |
| DM key fallback test | ✅ 已存在 |
請 re-review。感謝。
4 participants
fix: auto-capture smart extraction — issue #417 full resolution
Summary
Resolves issue #417 by implementing proper
extractMinMessagessemantics for theagent_endauto-capture hook. Supersedes PR #518 and PR #534.This PR fixes all blocking concerns raised in PR #534's review (rwmjhb):
currentCumulativeCountmonotonic increment — counter never resetspendingIngressTexts.delete()removed — pending texts accumulatepluginConfigOverridesspread — embedding always wins (needs comment)Changes
Fix #8 —
pendingIngressTextsdelete after consumption (index.ts, line ~2741)Under the REPLACE strategy, pending ingress texts were consumed but never removed from the map, causing re-processing on every subsequent
agent_end.Fix #9 —
currentCumulativeCountreset on successful extraction (index.ts, line ~2847)Counter grew monotonically forever — every
agent_endafter passing threshold triggered extraction. Resets inside the success block:Fix #4 —
pluginConfigOverridescomment (test/smart-extractor-branches.mjs)Fix #10 —
try-catcharoundextractAndPersist(index.ts, line ~2844)extractAndPersistcould throw on network errors or LLM timeouts. Without protection, an exception would propagate through the hook and potentially crash the entire plugin.Behavior on failure: counter is NOT reset (Fix #9), so the same message window will re-accumulate and retry on the next
agent_end.Testing
test/strip-envelope-metadata.test.mjs: envelope format mismatch in test environmenttest/smart-extractor-branches.mjs: Windows encoding issue with Chinese test datanpx tsc --noEmitpassesBreaking Change
The extraction trigger now functions as a sliding window: after a successful extraction, the counter resets and a new accumulation period begins. Previously, every
agent_endafter threshold would trigger extraction indefinitely. This is the intended semantic — the old behavior was wasteful and potentially harmful.Changelog
Code Review (OpenCode adversarial review)
pendingIngressTexts.delete)eligibleTexts.length > previousSeenCountmay skip new texts when context shrinks (requires unusual state to trigger).catch(() => {})— code quality onlyOpenCode conclusion: Fix #9 has a non-blocking edge case; Fix #8 and Fix #10 are correct.
Closes #518
Closes #534
Closes #417