feat: add resolvedAt + filterByResolved for reflection items (#447)

[jlin53882]

狀態更新 (2026-04-13)

修復已搬到新 PR：#595

這個 PR 現在關閉，由 PR #595 繼續。

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ec610e71fa

ℹ️ 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".

[chatgpt-codex-connector]

Prevent legacy fallback from reviving resolved reflection items

Passing unresolvedItemRows into buildInvariantCandidates/buildDerivedCandidates re-enables the legacy fallback path whenever all item rows for a slice are resolved. In that case itemCandidates.length becomes 0, so those helpers read from legacyRows, which still contain the original lines from memory-reflection entries and have no resolvedAt signal. In the default write path (which still stores combined legacy rows), resolved items can therefore reappear in recall output, defeating the suppression behavior introduced by this commit.

Useful? React with 👍 / 👎.

[jlin53882]

回覆：Prevent legacy fallback from reviving resolved reflection items

分析正確，感謝找到這個 edge case。

修正內容

在 loadAgentReflectionSlicesFromEntries 中加入 hasItemRows 判斷：

// Filter out resolved items — passive suppression for #447
const unresolvedItemRows = itemRows.filter(({ metadata }) => metadata.resolvedAt === undefined);

// If there were item rows but all are resolved, suppress to prevent legacy
// fallback from reviving them.  If there were NO item rows at all, allow
// legacy fallback to run (backward compatibility for stores that predate
// itemized reflection rows).
const hasItemRows = itemRows.length > 0;
if (hasItemRows && unresolvedItemRows.length === 0) {
  return { invariants: [], derived: [] };
}

邏輯

情況	行為
item rows 存在，但全部 resolved	return empty slices（阻擋 legacy fallback）
item rows 不存在（只有 legacy rows）	允許 legacy fallback（backward compatibility）

新增測試

第四個測試覆蓋 all-resolved + legacy rows 並存：

✔ returns empty slices when all invariant items are resolved to prevent legacy fallback

測試結果

36/37 通過。唯一失敗的是預先存在的 defaults to systemSessionMemory when neither field is set，與本次修改無關。

[app3apps]

This fix is too broad.

In loadAgentReflectionSlicesFromEntries() (src/reflection-store.ts:263), the new early return for hasItemRows && unresolvedItemRows.length === 0 clears all reflection output, not just the resolved item rows. That also suppresses unrelated legacy memory-reflection rows that may still exist in mixed/upgrade-era stores.

I reproduced it with:

• one resolved memory-reflection-item
• one older legacy memory-reflection row containing an unrelated derived line

Current result: { invariants: [], derived: [] }

So this avoids reviving resolved content through legacy fallback, but it also drops unrelated legacy advice. The new test covers the “don’t revive resolved text” case, but not this mixed-data case.

[jlin53882]

Review comment addressed. Here is a summary of the two fixes:

Fix 1: Content-aware early-return guard

The simple !hasLegacyRows guard was replaced with a content-aware check:

1. Build a Set of normalized texts from all resolved invariant/derived items
2. Check whether legacy rows contain any content NOT already covered by the resolved set
3. Only suppress when all items are resolved AND (no legacy rows OR all legacy content is pure duplicates of resolved items)

This correctly handles the P1 issue: in the default configuration (writeLegacyCombined !== false), legacy combined rows are co-written alongside every item row. The new guard prevents these duplicates from reviving just-resolved advice.

Fix 2: Additional test coverage

• P1 test case: suppresses legacy rows when all legacy content duplicates already-resolved items
• Existing test renamed to clarify scope: returns empty slices when ALL invariant items are resolved AND there are NO legacy rows

Test results

38 pass / 1 pre-existing fail (the failing test is unrelated to this PR)

Commit: 4dd05fc

[jlin53882]

Review comment addressed. Here is a summary of the two fixes:

Fix 1: Content-aware early-return guard

The simple !hasLegacyRows guard was replaced with a content-aware check:

1. Build a Set of normalized texts from all resolved invariant/derived items
2. Check whether legacy rows contain any content NOT already covered by the resolved set
3. Only suppress when all items are resolved AND (no legacy rows OR all legacy content is pure duplicates of resolved items)

This correctly handles the P1 issue: in the default configuration (writeLegacyCombined !== false), legacy combined rows are co-written alongside every item row. The new guard prevents these duplicates from reviving just-resolved advice.

Fix 2: Additional test coverage

• P1 test case: suppresses legacy rows when all legacy content duplicates already-resolved items
• Existing test renamed to clarify scope: returns empty slices when ALL invariant items are resolved AND there are NO legacy rows

Test results

38 pass / 1 pre-existing fail (the failing test is unrelated to this PR)

Commit: 4dd05fc

[AliceLJY]

Review: PR #483 — resolvedAt + filterByResolved for reflection items (#447)

Clean, minimal implementation that solves the core problem from #447.

Codex P1 assessment — "Prevent legacy fallback from reviving resolved reflection items"

Valid concern, and the author has already addressed it in the current code. The Codex comment warned that passing unresolvedItemRows (which can be empty when all items are resolved) into buildInvariantCandidates/buildDerivedCandidates would trigger the legacy fallback path, reviving resolved content from memory-reflection entries.

The fix is present: the code builds resolvedInvariantTexts/resolvedDerivedTexts sets from resolved items, then checks whether legacy rows contain any content that is NOT a duplicate of resolved items. The shouldSuppress logic (lines ~285-292) returns early with empty slices when all item rows are resolved AND legacy rows only contain duplicates. This correctly prevents the legacy fallback from reviving resolved advice.

Code review findings

1. Schema addition is clean — resolvedAt, resolvedBy, resolutionNote are all optional fields on ReflectionItemMetadata. No migration needed, backward compatible.

2. Filter logic is correct — resolvedAt === undefined as the unresolved check works because the field is genuinely absent (not set to null or 0). The metadata type enforces this via the optional ? modifier.

3. Test coverage is thorough — 6 new test cases covering:

   • Invariant suppression
   • Derived suppression
   • Unresolved pass-through
   • All-resolved + no legacy → empty output
   • Legacy duplicate suppression (the P1 fix)
   • Legacy with unique content NOT suppressed alongside resolved items

4. Edge case handled well — The "does NOT suppress legacy rows when resolved items exist alongside unrelated legacy rows" test confirms that unique legacy advice still surfaces even when all item rows are resolved.

5. Minor note — normalizeReflectionLineForAggregation is used for duplicate detection between resolved items and legacy rows. This is the right function since it strips noise before comparison, reducing false negatives.

LGTM. The memory_reflection_resolve tool (noted as follow-up) is correctly deferred.

[rwmjhb]

Review Summary

Useful feature (64%) — stale reflection items polluting context for 45 days is a real UX problem. The implementation has a legacy fallback gap that needs fixing.

Must Fix

1. Resolved content revives through legacy fallback — when a reflection item is marked resolved but its legacy counterpart (co-written row with unique lines) still exists, loadAgentReflectionSlicesFromEntries() picks up the legacy text and re-injects it. The resolvedSet filter only checks item.resolvedAt on the primary path, not on legacy deduplication.

2. Reflection tests not executed by CI — the new test file is not captured in the verification log. Confirm the test runner discovers and executes these tests in the merge-gating pipeline.

3. Rebase required — stale_base=true. Has main diverged in the reflection-store area?

Questions

• The write-path tool (memory_reflection_resolve) is deferred — without it, resolvedAt can only be set programmatically. Is there a tracking issue for the follow-up?
• Issue #395 — Reflection Items Lack Resolution Mechanism #447 asks whether resolving an item should auto-resolve all items with the same strictKey. Is this intentionally deferred?

Nice to Have

• Test for mixed resolved+legacy scenario doesn't use a co-written legacy row, so the legacy fallback gap (F2) is untested
• sanitizeInjectableReflectionLines applied to item text but not to legacy invariants in duplicate detection — asymmetric sanitization

Fix the legacy fallback leak and confirm test CI coverage, then this is mergeable.

[jlin53882]

Fix 1 回覆：Legacy Fallback Bug 已在 4dd05fcec 修復

問題分析

Bot P1 comment 的擔憂是對的——在較舊的 commit (ec610e7) 中：

// ❌ 較舊版本：所有 item 都 resolved 時，unresolvedItemRows = []
// buildInvariantCandidates([]) → itemCandidates.length === 0
// → fallback 到 legacyRows → resolved 內容復出
const invariantCandidates = buildInvariantCandidates(unresolvedItemRows, legacyRows);

修復方式

4dd05fcec（最新 commit）已加進 shouldSuppress 邏輯，在走 legacy fallback 前先判斷：

// ✅ 最新版本（4dd05fcec）
// 先算 shouldSuppress——當所有 item 都 resolved 且 legacy 只含 duplicate 時，直接回傳空
const shouldSuppress =
  hasItemRows &&
  unresolvedItemRows.length === 0 &&
  (!hasLegacyRows || (!legacyHasUniqueInvariant && !legacyHasUniqueDerived));

if (shouldSuppress) {
  return { invariants: [], derived: [] }; // 直接 early return，不走 legacy fallback
}

const invariantCandidates = buildInvariantCandidates(unresolvedItemRows, legacyRows);

Nice To Have 修復

Asymmetric sanitization：Bot 指出 legacy invariants 沒做 sanitize，已在 4dd05fcec 修復——legacyHasUniqueInvariant 現在對 legacy 內容也做 normalizeReflectionLineForAggregation()，和 resolved item text 處理一致。

---

Fix 2：本地測試結果

測試環境：node --test test/memory-reflection.test.mjs

memory reflection: 38 pass / 1 fail

✅ 新增測試全部通過：
  ✔ suppresses resolved invariant items from recall output
  ✔ suppresses resolved derived items from recall output
  ✔ passes unresolved items through unchanged when no resolvedAt is set
  ✔ returns empty slices when ALL invariant items are resolved AND there are NO legacy rows
  ✔ suppresses legacy rows when all legacy content duplicates already-resolved items (P1 fix)
  ✔ does NOT suppress legacy rows when resolved items exist alongside unrelated legacy rows

❌ Pre-existing failure（與本 PR 無關）：
  ✖ defaults to systemSessionMemory when neither field is set
  → 這個測試在 merge 前就失敗，與 resolvedAt 實作無關

---

Fix 3：關於 Questions

memory_reflection_resolve tool 的追蹤 issue 將在獨立的 follow-up PR 中處理（見 Issue #TBD）。

strictKey 連動 resolved 的設計決策也在同一 issue 中討論。

[jlin53882]

Update

Follow-up issue 已開：#558 — feat: add memory_reflection_resolve tool
包含：

• memory_reflection_resolve API 設計
• strictKey 連動 resolved 的設計決策（三個選項）
• cascade 語意建議

等待維護者回覆設計確認後再實作。

[jlin53882]

@rwmjhb

Hi, sorry to bother you! I've addressed all the items from your review:

1. Legacy fallback bug — shouldSuppress logic was added in 4dd05fcec. Explained in detail in my previous comment.
2. CI test coverage — local test results: 38 pass / 1 fail (the failing test is a pre-existing failure unrelated to this PR). All 6 new tests pass.
3. Rebase — done, rebaseable: true.
4. Nice-to-have fixes — co-written legacy row test and asymmetric sanitization are both covered in 4dd05fcec.

Follow-up issue for memory_reflection_resolve tool: #558

The PR is ready for your re-review whenever you have time. Thanks!

[jlin53882]

CI cli-smoke failure 分析（#483 附帶、非本次 PR 導致）

失敗的測試：test/update-consistency-lancedb.test.mjs

• 位置：:84、:145、:182
• 錯誤：Promise resolution is still pending but the event loop has already resolved
• failureType: cancelledByParent（parent 先失敗，subtest 被 cancel）

判定：flaky test，非 regression

檢查點	結論
PR #483 修改的檔案	reflection-item-store.ts、reflection-store.ts、test/memory-reflection.test.mjs
update-consistency-lancedb.test.mjs 在修改清單中？	❌ 不在
本次改動涉及的程式碼區塊	reflection resolved / suppressed，與 store update consistency 無關

該測試在 master branch 上本身就有 race condition 問題（常見於 Node.js 20→24 过渡期的并发测试），這次失敗屬於預期外的 flaky test。

建議：排除 flaky test 後可以 merge，或由 maintainer 確認後忽略該 check。

[jlin53882]

P2 Bug Fix — Per-Section Legacy Filtering

發現的問題

對抗式 Code Review（Codex）發現了一個 P2 Bug：

Bug 情境：當 Section A（Invariants）全 resolved、Section B（Derived）有 unique legacy content 時：

• shouldSuppress = false（因為 B 有 unique content）
• buildInvariantCandidates([], legacyRows) 走 legacy fallback
• 返回所有 legacy invariants（包括已 resolved 的）→ Bug！

修復內容

在 call site（src/reflection-store.ts）新增 per-section 過濾：

// 只傳有 unique invariant content 的 legacy rows
const invariantLegacyRows = legacyRows.filter(({ metadata }) =>
  toStringArray(metadata.invariants).some(
    (line) => !resolvedInvariantTexts.has(normalizeReflectionLineForAggregation(line))
  )
);
// 只傳有 unique derived content 的 legacy rows
const derivedLegacyRows = legacyRows.filter(({ metadata }) =>
  toStringArray(metadata.derived).some(
    (line) => !resolvedDerivedTexts.has(normalizeReflectionLineForAggregation(line))
  )
);
const invariantCandidates = buildInvariantCandidates(unresolvedItemRows, invariantLegacyRows);
const derivedCandidates = buildDerivedCandidates(unresolvedItemRows, derivedLegacyRows);

新增測試

測試	情境
invariants resolved + derived has unique legacy: only derived passes	Invariants resolved，Derived 有 unique legacy
derived resolved + invariants has unique legacy: only invariants passes	Derived resolved，Invariants 有 unique legacy

Codex 對抗式確認

✅ Codex 確認：「I did not find a discrete regression or missed edge case in this fix.」

測試狀態

• 39 tests pass
• 1 pre-existing failure（sessionStrategy defaults，與本 PR 無關，在 main 分支已存在）

[jlin53882]

CI cli-smoke 失敗說明

CI 中的 cli-smoke job 失敗與本 PR 變更無關，是 Issue #590 的 regression：

Issue #590：cli-smoke test failing: missing store.count() mock (regression from PR #582) — OPEN

根因

PR #582（fix: skip 75ms retry when store is empty）新增了 countStore() 參數傳入 retrieveWithRetry()，但 test/cli-smoke.mjs 的 mock store 沒有實作 count() 方法。導致 store.count 回傳 undefined → 整個 retrieveWithRetry 進 catch block → recallResult.details.count 變成 undefined。

驗證

此問題在 master 分支同樣存在（run 24304669192），非本 PR 引入。

建議

建議 maintainer 先確認 Issue #590 的修復方向，再合併本 PR。兩者獨立，無 blocking dependency。

[jlin53882]

@AliceLJY PR #483 已包含 P1 + P2 修復及新增測試。Codex 對抗式 review 確認無其他問題。CI cli-smoke 失敗是 Issue #590 的 regression（非本 PR 造成，master 也有同樣失敗）。其餘所有 jobs 均 pass。請確認後 merge，謝謝！

[jlin53882]

I have created a clean version of this PR with targeted diff:

This new PR has clean diff:

• src/reflection-store.ts: +62/-2 lines only
• src/reflection-item-store.ts: schema fields (unchanged)
• 5 new tests added, all passing

The original PR #483 has 2307/2169 diff due to formatting issues. Please consider:

1. Closing feat: add resolvedAt + filterByResolved for reflection items (#447) #483
2. Or changing the branch from eature/reflection-resolved-at to jlin53882:fix/p2-final

The new branch contains the same P1+P2 fixes with clean, surgical changes.

[jlin53882]

替代 PR 已建立：

這個 PR 使用 jlin53882:fix/p2-final branch，diff 乾淨（+62/-2 行）。請關閉 #483 或改用 #594。

[jlin53882]

更新

已重建乾淨的 PR：#595

Diff 現在只有 +70/-2，2 個檔案（原本的 +1709/-1195 是因為包含太多歷史 commit）。

變更內容

• \src/reflection-item-store.ts\：+6 行（resolvedAt, resolvedBy, resolutionNote 欄位）
• \src/reflection-store.ts\：+66/-2 行（Per-section legacy filtering）