fix: Issue #675 #676 complete batch mode implementation#782
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: d1611572aa
ℹ️ 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 (inv.bulkIndex !== undefined && inv.bulkIndex < bulkResults.length) { | ||
| const newEntryId = bulkResults[inv.bulkIndex].id; |
There was a problem hiding this comment.
When any earlier queued create entry is invalid (for example the embedding-failure path queues vector || []), MemoryStore.bulkStore filters it out before assigning IDs (src/store.ts 532-535), so bulkResults is a compressed array. In that case this bulkIndex can point at the wrong persisted memory or fall past the end, leaving the old memory with a missing or incorrect superseded_by after it is invalidated. Consider filtering/validating createEntries before recording indices, or carrying an explicit placeholder-to-result mapping from bulkStore.
Useful? React with 👍 / 👎.
PR 782 Fixes Follow-upFixed Issues
P0 Fix Details
P1 Fix Details
Not Fixed (Known Limitations)
Commit: d4ab579 |
PR 782 Known LimitationsDocumented Limitations
Why These Are Acceptable
Not Blockers
These are documented here for transparency. The PR can be merged with these known limitations documented. |
Unit Test Coverage for PR #782Add unit tests for New tests:
|
| Test | Verifies |
|---|---|
batch mode: uses bulkStore for new entry and calls update to invalidate old entry |
supersede decision → bulkStore creates new + update sets superseded_by on old |
batch mode: preserves original valid_from when invalidating old entry |
Invalidated entry retains original valid_from (history preserved) |
standalone path (no existing entry): uses bulkStore (batch mode) |
Empty store → create → bulkStore (not store) |
skips contradiction when getById returns null — no entry created, no update |
contradict + match_index: 1 + empty topSimilar → degrades to create (defensive) |
bulkStore succeeds, update fails: deletes new entries (rollback) |
update throws → rollback deletes entries from bulkStore |
Key behavioral findings
1. Destructive-decision guard
When topSimilar is empty and match_index points beyond the array, hasValidIndex = false triggers decision degradation — supersede/contradict becomes create. Prevents spurious invalidations against empty result sets.
2. Decision routing by category
handleSupersede vs handleContradict determined by category ∈ TEMPORAL_VERSIONED_CATEGORIES (preferences | entities). Non-TEMPORAL_VERSIONED categories route to handleContradict.
3. Rollback behavior
When bulkStore succeeds but update fails, only new entries (from bulkStore) are deleted. stats.rolledBack is not set by production code — rollback is logged but no stat emitted.
Execution
node --test test/pr678-pr723-batch-invalidation.test.mjs
# → # tests 5, # pass 5, # fail 0CI
Registered in scripts/ci-test-manifest.mjs (core-regression group). Branch: jlinfork/test/pr-782-unit-tests.
jlin53882
force-pushed
the
fix/pr-678-analysis
branch
from
d4ab579 to
be1ef34
Compare
|
Review finding: P1: CI manifest drops unrelated regression tests The CI manifest change removes several existing baseline tests while adding the new extraction/batch tests: |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
Requesting changes because this branch currently changes scripts/ci-test-manifest.mjs in a way that removes unrelated baseline coverage. The PR should only add its own PR-specific tests and must preserve all existing master entries.
Specifically, the manifest diff drops independent regression tests such as:
test/issue-690-cross-call-batch.test.mjstest/issue606_sdk-migration.test.mjstest/infer-provider-from-baseurl.test.mjstest/is-recall-used.test.mjstest/tier1-counters.test.mjs
Please rebase onto latest origin/master, restore all current master manifest entries, and only add this PR's necessary test entry/entries. After resolving, please run:
npm run test:packaging-and-workflow
npm run test:core-regressionThe branch should also be conflict-free and green before re-review.
… batch mode implementation - Tests for handleSupersede batch mode (bulkStore + update invalidation) - Tests for decision-degradation guard (contradict with invalid match_index → create) - Tests for rollback behavior when update fails (delete new entries from bulkStore) - Fix src/smart-extractor.ts to PR CortexReach#782 commit d161157 (1844 lines, 4 InvalidateEntry) - Register test in CI manifest (core-regression group) - Use jiti for TypeScript import in test files
jlin53882
force-pushed
the
fix/pr-678-analysis
branch
from
be1ef34 to
ebd0bc3
Compare
Restore store.ts to origin/master - P3 async file lock (Issue CortexReach#763) was inadvertently included during rebase and is not part of the PR CortexReach#782 scope (Issues CortexReach#675 CortexReach#676 batch mode implementation).
- Add onExtractionValidationFailed to ExtractPersistOptions interface
- Implement countBefore/countAfter validation in extractAndPersist()
- Remove T4 (compactor race test — cannot be reliably tested)
- Add ExtractionValidation type import to smart-extractor.ts
Validation logic:
- countBefore = store.count() before bulkStore
- countAfter = store.count() after bulkStore
- actual = countAfter - countBefore
- expected = createEntries.length
- If actual !== expected, invoke callback with { expected, actual, mismatch, sessionKey }
- Callback is optional (fail-silent if omitted)
CI 分析cli-smoke 失敗：import-markdown.test.mjs此測試失敗與本 PR 無關：
本 PR 修改的範圍src/smart-extractor.ts — 實作 onExtractionValidationFailed callback core-regression 測試結果✅ 通過 — extraction-validation.test.mjs 5/5 測試全部通過 cli-smoke 的 import-markdown.test.mjs 失敗應在另一個 PR 中追蹤修復。 |
rwmjhb
left a comment
rwmjhb
left a comment
There was a problem hiding this comment.
PR #782 Review: fix: Issue #675 #676 complete batch mode implementation
Verdict: REQUEST-CHANGES | 6 rounds completed | Value: 43% | Size: XL | Author: jlin53882
Value Assessment
Problem: The PR targets lock-contention and consistency problems in memory extraction batch mode, especially supersede flows that still bypass batching or need deferred invalidation of older memories. It also attempts to validate whether bulk extraction writes actually persist the expected number of entries.
| Dimension | Assessment |
|---|---|
| Value Score | 43% |
| Value Verdict | review |
| Issue Linked | true |
| Project Aligned | true |
| Duplicate | false |
| AI Slop Score | 2/6 |
| User Impact | medium |
| Urgency | medium |
Scope Drift: 4 flag(s)
- Issue #693 extraction validation callback and ExtractionValidation type are not part of the #675/#676 problem statement.
- The PR body claims index.ts regex fallback changes, but the observed diff contains no index.ts change.
- The author comment says handleContradict restores old behavior for missing targets, while src/smart-extractor.ts now returns early and skips creating the contradiction entry.
- dedup-false-alarm.test.mjs validates bulkStore internals rather than the supersede batch-mode bug directly.
AI Slop Signals:
- PR description claims index.ts changes, but index.ts is absent from the diff.
- The PR title and body focus on Issues #675 #676, while the diff adds Issue #693 validation API and tests.
Open Questions:
- Are Issues #675 and #676 acknowledged by maintainers beyond the provided issue text, since labels/assignment/comments are not shown?
- Should the Issue #693 extraction validation callback be split into its own PR?
- Is Issue #675 already fully handled by PR #723, leaving this PR to focus only on #676 and batch invalidation?
- Should handleContradict preserve the old orphan-entry behavior or intentionally skip when getById returns null?
- Is countBefore/countAfter validation acceptable under concurrent writes or background cleanup?
Summary
The PR targets lock-contention and consistency problems in memory extraction batch mode, especially supersede flows that still bypass batching or need deferred invalidation of older memories. It also attempts to validate whether bulk extraction writes actually persist the expected number of entries.
Evaluation Signals
| Signal | Value |
|---|---|
| Blockers | 0 |
| Warnings | 0 |
| PR Size | XL |
| Verdict Floor | approve |
| Risk Level | normal |
| Value Model | codex |
| Primary Model | codex |
| Adversarial Model | claude |
Must Fix
- F1: Changed source does not type-check
- F2: Supersede backfill still depends on unstable bulk result index
Nice to Have
- F3: Missing contradiction target is counted as created
- F4: Validation callback can abort post-bulk invalidation
- F5: Count-based write validation is not scoped to this extraction
- MR1: PR claims to fix #675 but contains no change to the regex-fallback path
- MR2: dedup-false-alarm test's 'cosine 0.95' premise is mathematically false
- MR3: ci-test-manifest.mjs fully rewritten for a 3-entry addition; missing trailing newline
- MR4: stats.rolledBack is incremented only when rollback FAILS, never on success
Recommended Action
Author should address must-fix findings before merge.
Reviewed at 2026-05-21T08:36:06Z | 6 rounds | Value: codex | Primary: codex | Adversarial: claude
3 participants
PR: Issue #675 + #676 完整實現 - 延續 PR #678 與 PR #723
摘要
本 PR 完整實現了 Issue #675（regex fallback bulkStore優化）和 Issue #676（handleSupersede batch mode），並基於 PR #723 的基礎上增加了完整的 batch invalidation 支援。
與 PR #678 的關係
與 PR #723 的差異
PR #723 的限制
PR #723 實現了：
但缺少：
本 PR 填補了這些缺口。
變更內容
index.ts
AUTO_CAPTURE_DEDUP_THRESHOLD = 0.90src/smart-extractor.ts
InvalidateEntryinterfacehandleSupersede：invalidateEntries參數extractAndPersist：invalidateEntries陣列queuedSupersedeMatchIdsSet（MR2 guard）processCandidate：supersedecase 加入 MR2 guardcontradictcase 加入 MR2 guard（當作為 supersede 時）handleContradict：測試
相關測試檔案：
test/invalidate-error-regression.test.mjstest/supersede-existing-found-bulk.test.mjs預期行為
關聯 Issue：