Add retrieval evals and supersede old master packs on distill - #127
Conversation
Roadmap #11 (retrieval evals) + master-pack proliferation fix. - createContextPack now supersedes older active packs for the same (user, namespace, pack_type) after inserting the new active pack: status-only ('archived'), reversible, no deletes, no memory_events touched, never crosses namespace or pack_type. Best-effort (a failure only adds a warning; the new pack is still returned). - New retrieval-eval tests (deterministic, in-memory client, no gated deps): AU query returns AU-only context; real_life query returns real_life-only; roadmap retrievable from real_life; AU canon retrievable from AU; MCP boundary adds ok/request_id/fallback_used; fallback_used=true on primary failure; payload cap respected; adaptive context surfaces the refreshed profile summary/confidence/preferences. - Supersede tests: new distill leaves exactly one active master pack for the namespace; older active master archived (not deleted); daily packs and the other namespace untouched. - Design note docs/people-map-whitelist-design.md: next people_map fix is a known-people whitelist (deterministic), NOT more stoplist; NER stays gated. No gated features touched; no destructive DB changes; AU/real_life separation preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28b9d2e233
ℹ️ 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".
| .eq("namespace", pf.data.namespace) | ||
| .eq("pack_type", result.data.pack_type) | ||
| .eq("status", "active") | ||
| .neq("id", result.data.id) |
There was a problem hiding this comment.
Serialize supersede updates for concurrent distills
When two distills for the same user/namespace/pack_type run concurrently, each insert returns an active row and then this update archives every other active row, including the other request's newly inserted pack. An interleaving like A insert, B insert, A archives B, then B archives A leaves no active pack while both callers returned an active result. Please make the supersede operation transactional/serialized or restrict it to rows older than the inserted pack so concurrent distills cannot retire each other.
Useful? React with 👍 / 👎.
| const warnings: string[] = []; | ||
| if (result.data.status === "active") { | ||
| const superseded = await (client.from("memory_context_packs") | ||
| .update({ status: "archived", updated_at: new Date().toISOString() }) |
There was a problem hiding this comment.
Audit archived context packs when superseding
If this update archives any prior active packs, it mutates persisted memory_context_packs rows without an audit entry for those archived record ids; the only audit write below records the newly inserted pack's id. That leaves the supersede status changes outside the repo's audit-backed memory-write trail and makes admin audit/readback unable to prove which packs were retired. Please append audit rows for the affected ids, ideally in the same transactional boundary as the archive update.
Useful? React with 👍 / 👎.
What
Roadmap #11 retrieval evals + the master-pack proliferation fix (supersede-on-distill), plus a design note for the real
people_mapfix.Changes
lib/services/memory-bridge-service.ts—createContextPacknow supersedes older active packs for the same(user, namespace, pack_type)right after inserting the new active pack. Status-only (archived), reversible, no deletes, nomemory_eventstouched, never crosses namespace or pack_type. Best-effort: a supersede failure only adds a warning; the new pack is still returned. (This is the canonical create path used by the MCP distill tool, the admin bridge, and the distill route.)tests/unit/retrieval-eval.test.ts(new, 8) — deterministic, in-memory client, no gated deps:ok/request_id/fallback_used;fallback_used=trueon primary failure;docs/people-map-whitelist-design.md(new) — the nextpeople_mapfix is a known-people whitelist (deterministic), not more stoplist; NER stays gated. Not implemented (blocked on a data/product decision — no static whitelist config exists yet).Schema changes
None —
status='archived'is an existing value; supersede is status-only.Verification
first-reviewed-memory-fixture→spawnSync npm ENOENT) is the pre-existing sandbox flake, unrelated.Risks / safety
memory_eventsmutation, no cross-namespace/pack_type effect.🤖 Generated with Claude Code