From dded8c9f3190d5e5efdbf1214dced5bd673037fa Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 12:14:14 +0800 Subject: [PATCH 1/9] refactor(context): extract SessionRecordsRepository (U1, plan 005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REVIEW §25 Phase 5 mechanical extraction. Behavior-preserving — the new ``SessionRecordsRepository`` mirrors the legacy filter shape, sort discipline, and limit ceiling exactly. U2 will tighten scope and replace the limit ceiling with cursor-based pagination. What moved (all from src/opencortex/context/manager.py to the new src/opencortex/context/session_records.py): - ``_load_session_merged_records`` → ``repo.load_merged`` - ``_load_session_directory_records`` → ``repo.load_directories`` - ``_session_layer_counts`` → ``repo.layer_counts`` - session_summary URI lookup (was inline in benchmark idempotent-hit branch) → ``repo.load_summary`` Two pure record-reading helpers also moved out of ``ContextManager`` into the same module so the repo could reuse them without circular imports: - ``ContextManager._record_msg_range`` (staticmethod) → module-level ``record_msg_range`` - ``ContextManager._record_text`` (staticmethod) → module-level ``record_text`` ContextManager constructs ``self._session_records`` once in ``__init__`` with a callable that resolves the active collection name (avoids capturing the whole orchestrator). All 12 legacy call sites across the lifecycle code (production conversation_end / context_commit plus benchmark ingest plus integrity logs) now go through the repo. Verification per plan §25.1: - 7 new unit tests in tests/test_session_records_repository.py cover happy path + edge cases + the two pure helpers. - Full lifecycle regression: 168/169 tests pass. Sole failure is pre-existing flaky `test_update_regenerates_fact_points_after_content_change` documented in project history. --- ...ingest-server-side-design-patterns-plan.md | 918 ++++++++++++++++++ .../2026-04-24-review-closure-tracker.md | 406 ++++++++ src/opencortex/context/manager.py | 168 +--- src/opencortex/context/session_records.py | 186 ++++ tests/test_session_records_repository.py | 213 ++++ 5 files changed, 1765 insertions(+), 126 deletions(-) create mode 100644 docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md create mode 100644 docs/residual-review-findings/2026-04-24-review-closure-tracker.md create mode 100644 src/opencortex/context/session_records.py create mode 100644 tests/test_session_records_repository.py diff --git a/docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md b/docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md new file mode 100644 index 0000000..a6dcf9c --- /dev/null +++ b/docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md @@ -0,0 +1,918 @@ +--- +title: "refactor: Benchmark ingest server-side design patterns (Service + Repository + DTO)" +type: refactor +status: active +date: 2026-04-25 +--- + +# refactor: Benchmark ingest server-side design patterns (Service + Repository + DTO) + +## Overview + +Closes the still-open server-side design-pattern phases from +`.context/compound-engineering/ce-code-review/20260424-152926-6301c860/REVIEW.md` +section 25 — Phases 3 (Service Layer), 5 (Repository / Gateway), and 6 +(DTO / Response Model). Phase 1 (Router Boundary) and Phase 2 (Facade) +were completed in PR #3 (commit `e50810b`); Phase 4 (Unit of Work) was +completed in PR #3+#4 (`_BenchmarkRunCleanup` + `RecompositionError`). +Phase 7 (Shared Adapter Helper) is intentionally out of scope — it lives +in `benchmarks/adapters/` and is orthogonal to the server-side Service / +Repository / DTO trio. A separate PR will pick it up. + +The shared driver across all three phases: `ContextManager.benchmark_ingest_conversation` +is a ~390-line method holding 6 distinct responsibilities (source persist, +segment normalize, entry build, leaf write + derive scheduling, recompose ++ summary, response build). Today it reaches into private repository +helpers, returns a bare `Dict[str, Any]`, and shares cleanup conventions +with the production lifecycle through manual co-ordination. The refactor +makes those boundaries explicit so the next reviewer can follow data flow +without reading 390 lines of mixed concerns. + +--- + +## Problem Frame + +Three open structural debts from the prior review (closure tracker +`docs/residual-review-findings/2026-04-24-review-closure-tracker.md`): + +- **R2-11 / F14**: SRP violation — `benchmark_ingest_conversation` packs + six responsibilities. Over the four-PR repair cycle the method grew + from ~100 to ~390 lines without changing shape; future incident + triage and feature work hit O(method-size) reading cost. +- **R2-12 / R4-P2-2**: Cleanup ownership scattered across four layers + (source, merged leaves, directory records via recomposition, summary). + `_BenchmarkRunCleanup` plus `RecompositionError` (PR #3+#4) cover the + data flow but the *ownership* of each URI lives in different methods + on the same God-class. Lifting the orchestration into a Service + centralises that ownership without disturbing the existing tracker. +- **R2-22 / R3-P-09**: Endpoint zero observability — no `StageTimingCollector` + hooks, no per-phase metrics, no LLM-call counts. The right surface + for these hooks is the Repository (query-time metrics) and the + Service (phase-time metrics). Without those surfaces, observability + has nowhere to land. + +Plus three open contract debts: + +- **R2-28 / R4-P2-10**: Bare `Dict[str, Any]` response. Adapters and + future orchestration layers can't depend on the shape; drift will + recur. +- **PE-2**: `_load_session_merged_records` / `_load_session_layer_counts` + silently truncate at 10 000 rows. Pre-existing footgun unblocked by + scope unification. +- **PE-6 / R3-RC-03**: `_load_session_merged_records` filter doesn't + carry `(tenant, user)` — papered over today by always passing + `source_uri` (which embeds tenant/user in the URI). Pre-existing. + +This plan executes the §25 Behavior-Preserving Refactor for the +server-side surface, scoped tightly per the §25.2 Abstraction Guardrails +("only Service, Repository, DTO; no Strategy / Abstract Factory"). + +Origin documents: +- Source review: `.context/compound-engineering/ce-code-review/20260424-152926-6301c860/REVIEW.md` (§25 specifically) +- Closure tracker: `docs/residual-review-findings/2026-04-24-review-closure-tracker.md` +- Prior plan that landed Phases 1/2/4: `docs/plans/2026-04-25-003-fix-benchmark-conversation-ingest-review-fixes-plan.md` + +--- + +## Requirements Trace + +### From REVIEW.md §25.1 Behavior-Preserving Refactor table + +- **R1 (Phase 5 Repository)**: Encapsulate `_load_session_merged_records`, + `_load_session_directory_records`, `_session_layer_counts`, and the + session_summary record lookup behind a single repository class. + Verification (per §25.1): "same session 不同 source/tenant 不混；超过 + limit 不静默截断." +- **R2 (Phase 5 scope discipline)**: All repository queries carry + `(tenant_id, user_id, source_uri)` scope where applicable. Closes + PE-6 / R3-RC-03 directly; reduces the foot-gun surface PE-2 sits on. +- **R3 (Phase 5 limit handling)**: Replace `limit=10_000` silent + truncation with explicit pagination (cursor-based) plus an overflow + guard that surfaces a structured warning rather than dropping rows. +- **R4 (Phase 6 DTO)**: Define `BenchmarkConversationIngestResponse` + and `BenchmarkConversationIngestRecord` Pydantic models in + `src/opencortex/http/models.py`. Verification: "Pydantic model + validation; adapter 只消费明确字段." +- **R5 (Phase 6 contract)**: DTO documents the `content` field's + hydration contract (R3-RC-06 lineage — "in-memory map captured at + write time, falls back to FS read") so future maintainers don't + rediscover the race. +- **R6 (Phase 3 Service extraction)**: Pull `benchmark_ingest_conversation` + out of `ContextManager` into `BenchmarkConversationIngestService`. + Six responsibilities become six private methods on the service. + Verification: "行为 golden test：相同输入幂等；相同 transcript 的 + records/msg_range 不变." +- **R7 (Phase 3 thin shim)**: `ContextManager.benchmark_ingest_conversation` + remains as a thin delegating method (or is removed entirely if no + internal caller depends on it). Choice resolved in U5. +- **R8 (Phase 3 cleanup ownership)**: All cleanup-tracker registration + happens inside the Service. `RecompositionError` flow continues to + surface partial directory URIs but they land in the Service-scoped + tracker, not a `ContextManager`-scoped one. + +### From §25.2 Abstraction Guardrails + +- **R9**: Service Layer is justified only because the method has + outgrown its host (391 lines, 6 responsibilities) — extraction + threshold met per the guardrail's "only when … extraction is + warranted." +- **R10**: Repository must serve ≥2 call sites — verified: 7 + 3 + 2 + call sites today across `manager.py`. Threshold met. +- **R11**: DTO fixes the endpoint contract only; does NOT pollute + internal storage record shape (Repository methods continue returning + `Dict[str, Any]` records as today, since storage adapters speak + dicts). +- **R12**: No Strategy / Abstract Factory introduced. Service has no + variants; Repository has no variants; DTO has no inheritance. + +### From §25.1 Verification (carried into per-unit Test Scenarios) + +- **R13**: Behavior golden test: same input → identical response. + Same transcript hash → identical merged-leaf URIs and `msg_range`. +- **R14**: Production conversation lifecycle (`context_commit`, + `context_end`) regression must pass. Repository touches helpers + shared with `_run_full_session_recomposition` (called from + `context_end` for merge follow-up). +- **R15**: Each phase committable independently — phases land in U1 + / (U2) / U3 / U4 / U5 commits. + +--- + +## Scope Boundaries + +- Do NOT modify production `context_commit`, `context_end`, or + `_run_full_session_recomposition` semantics. Repository wraps + shared helpers; calling sites in production paths are updated + mechanically only. +- Do NOT introduce Strategy / Abstract Factory / Builder / Visitor + patterns. Only Service + Repository + DTO. (§25.2 hard constraint.) +- Do NOT change endpoint URL, error codes, request shape, response + field set, or response semantics. Behavior-preserving refactor. +- Do NOT add observability (StageTimingCollector, metrics) in this + plan. The Service + Repository structure are the *surface* on + which a future PR can hang those hooks; landing the surface is + a separate concern from wiring the metrics. R2-22 stays open. +- Do NOT migrate `_benchmark_ingest_direct_evidence` to a separate + Service — it dispatches from inside `benchmark_ingest_conversation` + via the `ingest_shape` branch and stays in the same Service class + with both code paths exposed as service methods. +- Do NOT touch the orchestrator `add_batch` deferral (R2-08 / U12); + the Service uses `_orchestrator.add` per-leaf as today. + +### Deferred to Follow-Up Work + +- **§25 Phase 7** — Shared Adapter Helper (`benchmarks/adapters/conversation_mapping.py`): + separate PR after this lands. +- **R2-22 Observability**: Phase 6 / 7 follow-up once `StageTimingCollector` + conventions are decided across the wider orchestrator. +- **R2-08 / U12 add_batch**: Cross-cutting orchestrator change; needs + its own plan after this refactor settles. + +--- + +## Context & Research + +### Relevant Code and Patterns + +**Service Layer (Phase 3) target**: +- `src/opencortex/context/manager.py:1588-1978` — `benchmark_ingest_conversation` + body. 391 lines, 6 responsibilities. The extraction target. +- `src/opencortex/context/manager.py:1979-2042` — `_benchmark_ingest_direct_evidence`. + Sibling code path dispatched on `ingest_shape="direct_evidence"`. Move + alongside. +- `src/opencortex/context/manager.py:81-150` — `_BenchmarkRunCleanup` dataclass. + Already lives in this module; the Service uses it as-is. +- `src/opencortex/context/manager.py:81` — `RecompositionError`. Service + catches it and drains `created_uris` into the tracker. + +**Repository (Phase 5) call-site map**: +- `_load_session_merged_records`: 7 callers (lines 1669, 1894, 2233, + 2972, 3253, 3265 in `manager.py`, plus its own definition at 2148). +- `_load_session_directory_records`: 3 callers (1356, 3233, plus + definition at 2178). +- `_session_layer_counts`: 2 callers (3913 in `_finalize_session_end`, + plus definition at 2208). The benchmark response builder used to + call it but R2-04 removed that. +- Session summary record lookup: currently inline `_orchestrator._get_record_by_uri(_session_summary_uri(...))` + at `manager.py:1531-1534` (idempotent-hit branch). Promote to a + named repository method. + +**DTO (Phase 6) pattern reference**: +- `src/opencortex/http/models.py:116` — `MemorySearchResponse` (existing + Pydantic response model with optional fields, list of structured + result items, optional pipeline payload). Mirror this shape. +- `src/opencortex/http/models.py:325-329` — `MemorySearchResultItem` + fields list (uri, abstract, context_type, score, overview, content, + keywords, source_doc_id, etc.). Cherry-pick the fields the benchmark + response actually returns. +- `src/opencortex/http/models.py:298-360` — existing + `BenchmarkConversationIngestRequest` + `BenchmarkConversationMessage` + + `BenchmarkConversationSegment` (the request side; same module). + +**Existing facade conventions to mirror**: +- `src/opencortex/orchestrator.py` `MemoryOrchestrator.benchmark_conversation_ingest` + (PR #3 / U1) — public facade pattern. Will delegate to Service via + `self._context_manager._benchmark_ingest_service` (or similar + attribute) instead of directly to `_context_manager.benchmark_ingest_conversation`. +- `src/opencortex/http/admin_routes.py` `admin_benchmark_conversation_ingest` — + return type changes from `Dict[str, Any]` to the new DTO. FastAPI's + built-in serialization handles the rest. + +**Production lifecycle interactions**: +- `_run_full_session_recomposition` (manager.py around 2900) is called + by both `context_end` (production) and the Service (benchmark). Its + internal use of `_load_session_merged_records` becomes a Repository + call after Phase 5 lands. +- `_finalize_session_end` (line 3913 area) calls `_session_layer_counts` + for integrity logs. Repository's `session_layer_counts(...)` method + serves both this and any future caller. + +### Institutional Learnings + +- `docs/solutions/best-practices/single-bucket-scoped-probe-2026-04-16.md` — + Cited in plan 003. Repository scope discipline (`(tenant, user, source_uri)` + always passed) directly aligns with this entry's "scoped miss stays + scoped" principle. +- `docs/solutions/best-practices/memory-intent-hot-path-refactor-2026-04-12.md` — + DTO discipline aligns with this entry's "use phase-native envelopes, + don't revive flat fields." Future benchmark response extensions go + through the DTO, not bare dict additions. +- No existing entry for **Service / Repository / DTO extraction + pattern**. Strong `/ce-compound` candidate after merge. + +### External References + +External research skipped — Repository / Service / DTO are well-known +patterns; project has multiple existing examples (orchestrator facade, +context manager, MemorySearchResponse) to mirror. Codebase uses +`ContextManager`, `MemoryOrchestrator`, and various `*Adapter` classes +already; the Service / Repository pattern is the natural extension. + +--- + +## Key Technical Decisions + +- **Service constructor takes orchestrator handle, not full ContextManager.** + The Service needs `_orchestrator.add`, `_orchestrator._get_record_by_uri`, + `_orchestrator.remove`, plus the shared semaphores + (`_derive_semaphore`, `_directory_derive_semaphore`). It also needs + `_run_full_session_recomposition` and a few small helpers + (`_persist_rendered_conversation_source`, `_mark_source_run_complete`, + `_purge_torn_benchmark_run`, `_benchmark_recomposition_entries`, + `_aggregate_records_metadata`, `_merged_leaf_uri`, `_decorate_message_text`, + etc.) which still live on `ContextManager`. + + Two options: + - **(a)** Service stores a `ContextManager` reference and reaches in + for the helpers. Pragmatic; keeps the helpers where they are; risk: + perpetuates the God-class. + - **(b)** Move the helpers to `ContextManager` -> Service incrementally + in this plan. Cleaner; risk: balloons the diff. + + **Choice: (a)** for this PR. The Service holds a `manager: ContextManager` + reference and calls into it for shared helpers. Promotion of helpers + to the Service (or to a separate `BenchmarkRecomposeUtils` module) is + a follow-up. Rationale: §25.2 says "extract Service Layer only when + the host method has grown" — that justifies moving the orchestration, + not chasing every helper. + +- **Repository class location: `src/opencortex/context/session_records.py`.** + Sibling module to `manager.py` and `recomposition_types.py`. Avoids + circular import: Repository constructor takes a storage adapter and + collection-name resolver, not `ContextManager` itself. Implementation + details: + - `SessionRecordsRepository.__init__(storage, collection_resolver)` — + `collection_resolver` is a callable that returns the active collection + name (i.e., `_orchestrator._get_collection`). Avoids capturing the + whole orchestrator. + - Methods: `load_merged(session_id, *, source_uri, tenant_id=None, + user_id=None, limit=None, cursor=None)`, + `load_directories(session_id, *, source_uri, tenant_id=None, + user_id=None)`, `load_summary(tenant_id, user_id, session_id)`, + `layer_counts(session_id, *, source_uri=None, tenant_id=None, + user_id=None)`. Symmetric naming, all kwargs after `session_id`. + +- **Limit handling (R3): cursor-based pagination + overflow flag.** + Replace `limit=10_000` silent truncation with: + - Default page size 1 000. + - Repository methods that load "all rows for a session" (used in + response build) implement an internal loop that pages until exhausted. + - If the loop hits a configurable max-pages safety stop (default 50, + i.e., 50 000 rows), raise a `SessionRecordOverflowError` with the + cursor and current count, and let the caller decide. Benchmark + Service surfaces this as HTTP 507 (or 500 with structured detail). + - Pure overflow case is so rare on real benchmark data (LoCoMo + typical 37 leaves / conversation) that the production cost is + negligible; the limit-overflow guard is the correctness improvement. + +- **DTO field set matches current response exactly.** + Today's response (after Phase 1 work): + ``` + status: "ok" + session_id: str + source_uri: str | None + summary_uri: str | None + records: list[ {uri, abstract, overview, content, meta, abstract_json, + session_id, speaker, event_date, msg_range, + recomposition_stage, source_uri} ] + ``` + Plus `direct_evidence` path adds `ingest_shape: "direct_evidence"`. + + DTO mirrors this. `content` field carries a docstring documenting the + hydration semantics (in-memory map → FS fallback). No fields added, + no fields renamed, no fields removed in this PR. + +- **Service does NOT extract `_BenchmarkRunCleanup` or `RecompositionError`.** + Both already live as module-level classes in `manager.py:81-150` and + are imported by anyone who needs them. The Service imports both and + uses them unchanged. Moving them to the Service module is a cosmetic + follow-up. + +--- + +## Open Questions + +### Resolved During Planning + +- **Should ContextManager.benchmark_ingest_conversation become a thin shim or be removed?** + → Thin shim (one-line delegate). Removing it would break any + in-process test or maintenance script that bypasses the Service. The + shim costs nothing. +- **Should the Service own a separate cleanup-tracker class or reuse `_BenchmarkRunCleanup`?** + → Reuse. The tracker is well-tested, independent, and not coupled to + ContextManager. +- **Should Repository methods return Pydantic objects or `Dict[str, Any]`?** + → `Dict[str, Any]` (R11). Storage layer speaks dicts; Pydantic + serialization happens at the HTTP boundary only (DTO). +- **Should Repository scope discipline (R2) apply to production callers + of `_load_session_merged_records` (e.g., `_run_full_session_recomposition`)?** + → Yes. Production callers all have `(tenant, user, source_uri)` + available via `set_request_identity` contextvars + the `source_uri` + parameter they already pass. Adding scope to the Repository call + is a strict tightening with no behavior loss; in fact it closes + PE-6 (cross-tenant `session_id` collision footgun) for production + too. + +### Deferred to Implementation + +- Exact private method names inside the Service (e.g., `_persist_source` + vs `_ensure_source_versioned` vs `_write_source`). +- Whether `SessionRecordOverflowError` deserves a dedicated exception + class or can use `ValueError` with a structured message — defer to + the implementer; either is acceptable. +- Page size constant (1 000) and max-pages safety stop (50) — pick + during U2 based on what the test fixture exercises. +- Whether to inline the session_summary lookup in U1 or wait for U2 — + the helper is one line today; keeping it inline in U1 is fine. + +--- + +## High-Level Technical Design + +> *This illustrates the intended approach and is directional guidance for review, not implementation specification. The implementing agent should treat it as context, not code to reproduce.* + +``` + +-----------------------------------+ + | http/admin_routes.py | + | admin_benchmark_conversation_ | + | ingest(req) | + | -> calls orch.benchmark_ | + | conversation_ingest(...) | + | -> returns | + | BenchmarkConversationIngest | + | Response | + +-----------------+-----------------+ + | + v + +-------------------------+--------------------------+ + | orchestrator.MemoryOrchestrator | + | .benchmark_conversation_ingest(...) | + | (existing public facade — PR #3 / U1) | + | -> delegates to | + | self._context_manager._benchmark_ingest_service | + +-------------------------+--------------------------+ + | + v + +------------------------------------+-----------------------------------+ + | context/benchmark_ingest_service.py | + | class BenchmarkConversationIngestService | + | __init__(manager, repo) | + | async def ingest(session_id, tid, uid, segments, | + | include_session_summary, ingest_shape) | + | -> BenchmarkConversationIngestResponse | + | | + | private: | + | _persist_source(...) (uses manager helpers) | + | _normalize_segments(...) | + | _build_merged_leaf_entries(...) | + | _write_merged_leaves_with_derive(...) (uses cleanup tracker) | + | _recompose_and_summarize(...) (uses RecompositionError catch) | + | _build_response(...) (uses repo + DTO) | + | _ingest_direct_evidence(...) (alternate ingest_shape branch) | + +------+----------------------------+-------------------------+---------+ + | | | + v v v + +----------+----------+ +-------------+------------+ +--------+----------+ + | context/ | | context/manager.py | | http/models.py | + | session_records.py | | _BenchmarkRunCleanup | | BenchmarkConv. | + | SessionRecords | | RecompositionError | | IngestResponse | + | Repository | | (existing helpers used | | + IngestRecord | + | .load_merged(...) | | via manager param) | | (Pydantic) | + | .load_directories() | +---------------------------+ +-------------------+ + | .load_summary(...) | + | .layer_counts(...) | + +----------------------+ + ^ + | + | (production callers also use this repo) + | + +----------+-----------+ + | context/manager.py | + | _run_full_session_ | + | recomposition(...) | + | _finalize_session_end | + | (other 7-3-2 sites) | + +-----------------------+ +``` + +Key boundary properties: +- Admin route only knows about: orchestrator facade + DTO. +- Orchestrator facade only knows about: Service. +- Service only knows about: Repository, DTO, manager (for shared + helpers), `_orchestrator.add` / `.remove` / `_get_record_by_uri`, + cleanup tracker classes. +- Repository only knows about: storage adapter, collection name. +- ContextManager retains all helpers it owns today; Service borrows + them via the `manager` reference. + +--- + +## Implementation Units + +### Phase 5 — Repository / Gateway + +- [ ] U1. **Extract `SessionRecordsRepository` (mechanical move, behavior-preserving)** + +**Goal:** Create `src/opencortex/context/session_records.py` housing +`SessionRecordsRepository`. Move `_load_session_merged_records`, +`_load_session_directory_records`, `_session_layer_counts`, and the +session_summary record lookup off `ContextManager` and onto the +repository class. Update all 12 call sites to use +`self._session_records.(...)` — `ContextManager.__init__` +constructs a single repo instance once `_orchestrator` is set. + +**Requirements:** R1, R10, R14. + +**Dependencies:** None. + +**Files:** +- Create: `src/opencortex/context/session_records.py` +- Modify: `src/opencortex/context/manager.py` (delete 4 helper bodies; add `self._session_records` attribute; rewrite ~12 call sites) +- Test: `tests/test_session_records_repository.py` (new — golden parity tests) +- Test: existing `tests/test_e2e_phase1.py`, `tests/test_context_manager.py`, `tests/test_http_server.py`, `tests/test_locomo_bench.py`, `tests/test_benchmark_ingest_lifecycle.py` all must pass unchanged + +**Approach:** +- Repository constructor: `(storage, collection_resolver: Callable[[], str])`. The resolver is `lambda: orchestrator._get_collection()` — avoids capturing the orchestrator reference. +- Each method's body is the existing helper body, copy-pasted with `self._storage` / `self._collection()` substitutions. No filter-DSL changes in U1. +- ContextManager constructs the repo lazily on first access OR in `__init__` after orchestrator is set. (Ordering question — `_orchestrator` is set in `__init__` argument; verify by grep.) +- Call-site updates are mechanical: `await self._load_session_merged_records(session_id=..., source_uri=...)` becomes `await self._session_records.load_merged(session_id=..., source_uri=...)`. Method name change `_load_session_*` → `load_*`; underscore prefix dropped because the methods are now public on the repo. + +**Execution note:** Add a behavior parity test (`tests/test_session_records_repository.py`) that asserts repo methods return the same rows as the legacy helpers for a fixture, before deleting the legacy helpers. This locks the move. + +**Patterns to follow:** +- `ContextManager.__init__` attribute-set patterns for `self._derive_semaphore`, `self._directory_derive_semaphore` (instance-scoped state). +- `MemoryOrchestrator._get_collection` callable pattern. + +**Test scenarios:** +- Happy path: `repo.load_merged(session_id="s1", source_uri="opencortex://t/u/session/conversations/s1/source")` returns the same records as the legacy `_load_session_merged_records` for the same fixture. +- Happy path: `repo.load_directories(session_id="s1", source_uri="...")` parity with legacy. +- Happy path: `repo.layer_counts(session_id="s1")` parity with legacy. +- Happy path: `repo.load_summary(tenant_id="t", user_id="u", session_id="s1")` returns the existing summary record or None. +- Edge case: empty session — all four methods return empty list / zero counts / None. +- Edge case: nonexistent session_id — same. +- Integration: production lifecycle — run `tests.test_e2e_phase1` and `tests.test_context_manager` to confirm `_run_full_session_recomposition`, `_finalize_session_end`, and any benchmark path that uses repo all behave identically to before. + +**Verification:** +- `grep "_load_session_merged_records\|_load_session_directory_records\|_session_layer_counts" src/opencortex/context/manager.py` shows 0 hits (helpers fully removed). +- Repo test file passes; full lifecycle and benchmark suites pass with no regression. + +--- + +- [ ] U2. **Add scope discipline + pagination + overflow guard to Repository** + +**Goal:** Each repository method enforces `(tenant_id, user_id, source_uri)` +filter discipline — closing PE-6 / R3-RC-03 — and replaces +`limit=10_000` silent truncation with explicit pagination plus a +`SessionRecordOverflowError` when results exceed a safety stop. +Production callers automatically benefit because they already have +identity + source available. + +**Requirements:** R2, R3, R14. + +**Dependencies:** U1. + +**Files:** +- Modify: `src/opencortex/context/session_records.py` (extend method + signatures with `tenant_id` / `user_id` kwargs + paging logic + + overflow guard) +- Modify: `src/opencortex/context/manager.py` (call sites pass + identity from `set_request_identity` context where available; pass + None for legacy paths that genuinely don't have it) +- Modify: `src/opencortex/http/admin_routes.py` (catch new + `SessionRecordOverflowError` and surface as HTTP 507 with structured + detail) +- Modify: `src/opencortex/context/benchmark_ingest_service.py` (will + exist after U4 — for now, the Service-side wiring lands in U4 too; + in U2 only the manager-call-site updates land) +- Test: `tests/test_session_records_repository.py` (extend with scope + + pagination + overflow scenarios) + +**Approach:** +- Add `tenant_id: Optional[str] = None`, `user_id: Optional[str] = None` + kwargs to `load_merged`, `load_directories`, `layer_counts`. When + provided, filter narrows to that tenant/user. When `source_uri` is + set, that already implies tenant/user (URI structure), so explicit + tenant/user is redundant for those calls but harmless. For + `source_uri=None` calls, tenant/user becomes the strict filter, + closing PE-6. +- Pagination: internal `_paged_filter(...)` loops `storage.scroll` + with default page size 1 000. Methods that conceptually return + "all rows for this session" exhaust the scroll and assemble the full + list, raising `SessionRecordOverflowError` after a safety stop + (default 50 pages = 50 000 rows). +- Overflow class: `SessionRecordOverflowError(session_id, count, cursor)` + carries enough context to debug. HTTP 507 (Insufficient Storage — + closest semantically to "too much data to return") with detail + body containing `{reason, session_id, count_at_stop, hint}`. +- Repo's `load_summary` doesn't paginate (always 1 row). + +**Execution note:** Test scope discipline against an InMemoryStorage +fixture with two synthetic tenants having the same `session_id`. The +post-U2 repo must NOT mix them. + +**Patterns to follow:** +- Existing `storage.scroll(...)` API (signature in + `src/opencortex/storage/storage_interface.py` — verify during + implementation). +- `RecompositionError` (`src/opencortex/context/manager.py:81`) as the + named-exception precedent. +- 504 / 409 / 410 HTTP error envelope in `src/opencortex/http/admin_routes.py` + for the new 507. + +**Test scenarios:** +- Happy path: `repo.load_merged(session_id="s1", tenant_id="A", user_id="x")` returns only A/x records even if B/y has the same session_id. +- Happy path: `repo.load_merged(session_id="s1", source_uri="...")` works as before; explicit tenant/user kwargs harmless when source_uri is set. +- Happy path: 5 000-row fixture loads completely via internal pagination (5 page fetches, no truncation). +- Edge case: 0 rows — empty list returned, no overflow. +- Edge case: exactly at safety stop (50 000 rows) — returns all without raising. +- Error path: 50 001-row fixture raises `SessionRecordOverflowError` with `count == 50_000` and `cursor` set. +- Error path: HTTP layer maps `SessionRecordOverflowError` → 507 with structured detail. +- Integration: production `tests.test_context_manager` lifecycle test exercises a typical (not overflow) session — must pass identical to U1. + +**Verification:** +- All repo methods accept `tenant_id` / `user_id` kwargs; default `None` preserves U1 behavior. +- Cross-tenant `session_id` collision test fails on legacy; passes on U2 repo. +- Overflow test fires when a synthetic high-cardinality session is loaded. + +--- + +### Phase 6 — DTO / Response Model + +- [ ] U3. **Define `BenchmarkConversationIngestResponse` Pydantic models** + +**Goal:** Add `BenchmarkConversationIngestResponse` and +`BenchmarkConversationIngestRecord` to `src/opencortex/http/models.py`. +Document the `content` field's hydration semantics (in-memory map → +FS fallback per R3-RC-06). Models are added but not yet wired — the +admin route still returns `Dict[str, Any]` until U5. + +**Requirements:** R4, R5, R11. + +**Dependencies:** None (independent of U1/U2; can land in any order +relative to them, but easiest after U1 to avoid context-switching). + +**Files:** +- Modify: `src/opencortex/http/models.py` (append two new model classes + near the existing `BenchmarkConversationIngestRequest` definition) +- Test: `tests/test_http_models.py` (new — model validation tests) + OR extend `tests/test_http_server.py` with model-validation + assertions + +**Approach:** +- `BenchmarkConversationIngestRecord` mirrors today's per-record dict + shape exactly: `uri`, `abstract`, `overview`, `content`, `meta`, + `abstract_json`, `session_id`, `speaker`, `event_date`, `msg_range`, + `recomposition_stage`, `source_uri`. Field types match what + `_export_memory_record` returns today (`content: str` with hydration + doc, `meta: Dict[str, Any]`, `msg_range: Optional[List[int]]`, + `recomposition_stage: Optional[str]`, etc.). +- `BenchmarkConversationIngestResponse` mirrors today's top-level + response: `status: Literal["ok"]`, `session_id: str`, + `source_uri: Optional[str]`, `summary_uri: Optional[str]`, + `records: List[BenchmarkConversationIngestRecord]`. The + `direct_evidence` path adds `ingest_shape: Optional[Literal["direct_evidence"]] = None` + — keep optional so default `None` covers the merged_recompose path. +- Field docstrings carry the contract: `content` notes the hydration + rule; `summary_uri` notes that idempotent-hit returns the prior + run's summary URI when present. + +**Patterns to follow:** +- `src/opencortex/http/models.py:116` `MemorySearchResponse` (Pydantic + response with optional fields). +- `src/opencortex/http/models.py:88` `MemorySearchResultItem` (per-row + model with optional metadata fields). + +**Test scenarios:** +- Happy path: a representative current response dict (from a passing + `tests/test_http_server.test_04d` payload) validates against the new + model without modification. +- Happy path: a `direct_evidence` response dict validates with + `ingest_shape="direct_evidence"`. +- Edge case: empty records list validates. +- Edge case: `source_uri=None` and `summary_uri=None` both validate. +- Error path: response dict missing `status` field fails validation. +- Error path: `status="error"` (not in Literal) fails validation. + +**Verification:** +- Models exist; no admin-route or service code changes yet; tests pass. + +--- + +### Phase 3 — Service Layer + +- [ ] U4. **Extract `BenchmarkConversationIngestService`** + +**Goal:** Pull the body of `ContextManager.benchmark_ingest_conversation` +(391 lines) and `_benchmark_ingest_direct_evidence` into a new +`BenchmarkConversationIngestService` class. Six responsibilities split +into private methods on the service. The Service depends on the +Repository (U1+U2) and uses the existing `_BenchmarkRunCleanup` / +`RecompositionError` classes unchanged. + +**Requirements:** R6, R7, R8, R9, R12, R13. + +**Dependencies:** U1 (repo for response build), U3 (DTO for return +type — though U4 still returns dict and U5 wires the DTO; alternative +is U4 returns DTO directly. See "Approach" for the call.). + +**Files:** +- Create: `src/opencortex/context/benchmark_ingest_service.py` (new + module housing `BenchmarkConversationIngestService`) +- Modify: `src/opencortex/context/manager.py` (replace + `benchmark_ingest_conversation` body with a thin shim; + `_benchmark_ingest_direct_evidence` becomes a thin shim too OR is + deleted because only the Service-internal path calls it) +- Modify: `src/opencortex/context/manager.py` `__init__` to construct + `self._benchmark_ingest_service` after orchestrator + repo are ready +- Test: `tests/test_benchmark_ingest_service.py` (new — golden parity: + Service.ingest produces identical response to legacy method for the + same input) +- Test: existing `tests/test_benchmark_ingest_lifecycle.py` and + `tests/test_http_server.py` continue to pass without modification + +**Approach:** +- Service constructor: `(manager: ContextManager, repo: SessionRecordsRepository)`. + Takes the manager reference for the helpers that didn't move + (`_benchmark_recomposition_entries`, `_aggregate_records_metadata`, + `_merged_leaf_uri`, `_decorate_message_text`, `_persist_rendered_conversation_source`, + `_mark_source_run_complete`, `_purge_torn_benchmark_run`, + `_hydrate_record_contents`, `_export_memory_record`, etc.). +- Public method: `async def ingest(*, session_id, tenant_id, user_id, + segments, include_session_summary, ingest_shape) -> Dict[str, Any]`. + Returns dict for U4 (DTO wiring is U5); the dict shape is locked + by the existing tests. +- Six private methods, one per responsibility: + - `_persist_source(...)` — calls manager's + `_persist_rendered_conversation_source(..., enforce_transcript_hash=True)` + - `_normalize_segments(segments) -> List[List[Dict]]` — pure function + - `_build_merged_leaf_entries(normalized_segments) -> List[entries]` + — uses `manager._benchmark_recomposition_entries` and + `manager._build_recomposition_segments` + - `_write_merged_leaves_with_derive(segments, source_uri, cleanup, + content_map)` — the per-leaf write loop + derive task scheduling + + `await asyncio.gather(*tasks)` with sibling-cancel handler + - `_recompose_and_summarize(session_id, source_uri, + include_session_summary, cleanup)` — calls + `manager._run_full_session_recomposition(..., return_created_uris=True)`, + catches `RecompositionError` and drains URIs into cleanup, + optionally calls `manager._generate_session_summary` + - `_build_response(merged_records, source_uri, cleanup, + content_map) -> Dict[str, Any]` — uses repo + manager helpers +- The dispatch on `ingest_shape == "direct_evidence"` lives in + `Service.ingest`. The direct_evidence body becomes + `_ingest_direct_evidence(session_id, tenant_id, user_id, source_uri, + normalized_segments)` private method. +- Cleanup tracker (`_BenchmarkRunCleanup`) is constructed in + `Service.ingest` and threaded through `_write_merged_leaves_*` + and `_recompose_and_summarize`. Same compensation flow as today. +- The thin shim on `ContextManager.benchmark_ingest_conversation` + becomes: + ``` + return await self._benchmark_ingest_service.ingest( + session_id=session_id, tenant_id=tenant_id, user_id=user_id, + segments=segments, + include_session_summary=include_session_summary, + ingest_shape=ingest_shape, + ) + ``` + +**Execution note:** Add `tests/test_benchmark_ingest_service.py` with +a golden parity test BEFORE deleting the legacy method body. The +test feeds the same payload to legacy and Service code and asserts +byte-equal response dicts. Land that test, then refactor. + +**Patterns to follow:** +- `ContextManager.__init__` for the construction sequence. +- `MemoryOrchestrator.__init__` and its lazy-init pattern for the + Service if construction order matters. +- `_BenchmarkRunCleanup.compensate` (`manager.py:103-141`) — the + Service should call it in the same except handlers (CancelledError + + Exception) currently in `benchmark_ingest_conversation`. + +**Test scenarios:** +- Happy path: Service.ingest with a representative LoCoMo-shaped + payload returns the same response (top-level keys, record count, + per-record `msg_range`, `source_uri`, `summary_uri`) as the legacy + ContextManager method for the same input. +- Happy path: idempotent replay (same transcript hash) returns + identical response on second call (golden test for hash-versioning + preservation). +- Happy path: 409 conflict path still raises `SourceConflictError` + unchanged. +- Happy path: torn-replay path still purges and re-ingests (existing + `test_torn_prior_run_is_not_treated_as_idempotent` must pass + unchanged). +- Edge case: empty `normalized_segments` returns the same + empty-records short-circuit as today. +- Error path: monkeypatch `_run_full_session_recomposition` to raise + `RuntimeError` — Service catches via cleanup compensation, no + orphan records (existing `test_recompose_failure_cleans_up_merged_leaves` + must pass). +- Error path: `CancelledError` mid-derive-gather still triggers sibling + cancellation + cleanup (existing + `test_cancelled_error_propagates_after_cleanup` must pass). +- Integration: full `tests/test_benchmark_ingest_lifecycle.py` suite + passes without modification. +- Integration: `tests/test_http_server.test_04d/04e/04f/04g/04h/04i` + pass without modification (admin route still returns dict; DTO + comes in U5). +- Integration: production `tests/test_context_manager.py` and + `tests/test_e2e_phase1.py` pass — the manager-side helpers the + Service borrows are untouched. + +**Verification:** +- `wc -l src/opencortex/context/manager.py` drops by ~390 lines (the + benchmark_ingest_conversation + _benchmark_ingest_direct_evidence + bodies move into the Service). +- New service module is < 500 lines (with the 6 private methods + + direct_evidence + class scaffolding). +- `ContextManager.benchmark_ingest_conversation` is a one-line + delegate. +- All existing test suites pass identical to pre-U4 master. + +--- + +### Phase 6 (continued) — Wire DTO through the public path + +- [ ] U5. **Wire `BenchmarkConversationIngestResponse` through Service + admin route** + +**Goal:** `Service.ingest` returns `BenchmarkConversationIngestResponse` +instead of `Dict[str, Any]`. Admin route declares the DTO as the +response type. Orchestrator facade preserves the dict signature for +backward compat (returns `model.model_dump()` from the Service result) +or also returns the DTO — choose during implementation. + +**Requirements:** R4, R7. + +**Dependencies:** U3 (DTO defined), U4 (Service exists). + +**Files:** +- Modify: `src/opencortex/context/benchmark_ingest_service.py` (wrap + the current dict return in `BenchmarkConversationIngestResponse(**dict)` + or build the model directly) +- Modify: `src/opencortex/orchestrator.py` + `MemoryOrchestrator.benchmark_conversation_ingest` (return type + annotation update; possibly call `.model_dump()` to preserve dict + return for any non-HTTP caller — verify by grep that no in-process + caller depends on the dict) +- Modify: `src/opencortex/http/admin_routes.py` + `admin_benchmark_conversation_ingest` (return type annotation + update from `Dict[str, Any]` → `BenchmarkConversationIngestResponse`; + FastAPI handles serialization automatically) +- Modify: `src/opencortex/context/manager.py` thin shim returns the + DTO too (same change) +- Test: existing tests should pass without modification — FastAPI + serializes Pydantic to identical JSON the dict produced. If a test + asserts on a field absent from the DTO, fix the DTO to include it. + +**Approach:** +- Build the DTO once at the response construction site inside the + Service (replaces today's `return {...}`). Map `_export_memory_record` + output to `BenchmarkConversationIngestRecord(**row)` before placing + in the records list. +- Admin route `-> BenchmarkConversationIngestResponse:` — FastAPI's + default serializer emits identical JSON. +- Orchestrator facade decision (resolved during implementation): if + no in-process caller exists (verified by grep), return the DTO. If + any test or maintenance script expects a dict, return + `model.model_dump()`. Document the decision inline. + +**Patterns to follow:** +- `MemorySearchResponse` is returned directly from the search route + (`src/opencortex/http/server.py` `memory_search` route). FastAPI + serializes it. + +**Test scenarios:** +- Happy path: HTTP `test_04d` response JSON byte-equal to pre-U5 (the + DTO must round-trip the same field set). +- Happy path: HTTP `test_04e` (`direct_evidence`) JSON identical. +- Edge case: idempotent-hit response includes `summary_uri` correctly + (Pydantic serializes `Optional[str]` with `None` as JSON `null`). +- Error path: Pydantic validation rejects a malformed Service-side + build — surfaces as 500. (Defensive; should never fire if Service + honors the contract.) +- Integration: full `tests/test_benchmark_ingest_lifecycle.py` and + `tests/test_locomo_bench.py` pass with the DTO return type. + +**Verification:** +- Admin route handler signature shows `-> BenchmarkConversationIngestResponse:`. +- Service `ingest()` return type is the DTO. +- All HTTP tests pass without modification (JSON byte-equivalence). +- A grep for `Dict[str, Any]` in the benchmark code paths returns + zero hits except where genuinely needed (e.g., `meta` field type). + +--- + +## System-Wide Impact + +- **Interaction graph**: New module dependencies — `benchmark_ingest_service.py` + depends on `manager.py` (helpers), `session_records.py` (repo), + `http/models.py` (DTO), `_BenchmarkRunCleanup` + `RecompositionError` + (already in manager.py). No new HTTP routes; no new MCP tools. +- **Error propagation**: New `SessionRecordOverflowError` raised by repo, + caught at admin route → HTTP 507. `RecompositionError` still raised + by `_run_full_session_recomposition`, now caught inside Service + `_recompose_and_summarize` instead of inside the legacy method body. + All existing exception types (`SourceConflictError`, `CancelledError`, + generic `Exception`) preserved. +- **State lifecycle risks**: Cleanup tracker (`_BenchmarkRunCleanup`) + is constructed once per `Service.ingest` call — same scope as + today's `benchmark_ingest_conversation`. No new shared state. The + Repository's pagination loop is in-memory; if a request is cancelled + mid-load, the partial result is discarded — no orphan state. +- **API surface parity**: HTTP `POST /api/v1/admin/benchmark/conversation_ingest` + response shape unchanged. New 507 status code only for the genuine + overflow case (50 000+ records under one session — unrealistic on + benchmark data). Admin route URL, methods, request shape, request + validation all unchanged. +- **Integration coverage**: Production `context_commit` / `context_end` + lifecycle uses Repository methods after U1+U2. Mandatory regression: + `tests.test_e2e_phase1` + `tests.test_context_manager` (the + conversation lifecycle suite). U4 doesn't add new integration risk + — the Service is opt-in via `benchmark_conversation_ingest` only. +- **Unchanged invariants**: + - `POST /api/v1/admin/benchmark/conversation_ingest` URL, request + shape, response field set, response field semantics, error codes + (403/409/422/504/410) — all unchanged. + - `MemoryOrchestrator.benchmark_conversation_ingest` keyword-arg + signature unchanged. + - `ContextManager.benchmark_ingest_conversation` keyword-arg + signature unchanged (thin shim). + - Production `context_commit` / `context_end` lifecycle unchanged. + - MCP tool surface (11 tools per agent-native check) unchanged. + - All cleanup tracker compensation order unchanged. + +--- + +## Risks & Dependencies + +| Risk | Mitigation | +|---|---| +| **Repository scope discipline (U2) might break a production caller that doesn't have `tenant_id` / `user_id` available.** | Default kwargs to `None`; legacy callers preserve old behavior. Add scope explicitly only at known call sites. Production `context_end` flow has `set_request_identity` contextvars — tenant/user are always reachable via `get_effective_identity()`; that's the lookup path. | +| **Cursor-based pagination might surface a previously-hidden ordering bug** in the storage adapter. | Repo wraps `storage.scroll`; if scroll has ordering inconsistencies, U2 surfaces them as failing parity tests. Stop and fix at the storage layer if so. | +| **Service extraction (U4) might inadvertently change a response field.** | Golden parity test (`tests/test_benchmark_ingest_service.py`) asserts byte-equal response dict for representative payloads BEFORE the legacy method is replaced. Test must pass before the replacement lands. | +| **DTO wiring (U5) might break a downstream JSON consumer that depends on field-name typos or extra fields.** | `model_extra="allow"` (Pydantic default behavior) preserves any extra fields the response had; the DTO declares the documented set. Test parity is byte-equal JSON, not field-equal Python dict. If a typo escapes documentation, surface it during implementation. | +| **Helper coupling** — Service borrows ~10 helpers from ContextManager via a `manager` reference. Future ContextManager refactors might break the Service. | This is intentional per Key Technical Decisions choice (a). Promotion of helpers to the Service is an explicit follow-up in `Deferred to Follow-Up Work`. | +| **Refactor scope creep** — easy to start "while I'm in here, let me also fix R2-23 (duplicate fs.write_context) / PE-1 (rename _delete_immediate_families) / R3-RC-02 (cross-input-session merge)." | Plan scope is locked to §25 Phases 3+5+6. Additional fixes go in separate PRs per closure tracker action queue. | +| **Production lifecycle regression** — the Repository wraps helpers also called from `_run_full_session_recomposition`, `_finalize_session_end`, recall path. | U1 verification mandates the production lifecycle test suite passes. U2 verification ditto. Any regression is caught before merge. | + +--- + +## Documentation / Operational Notes + +- **No CHANGELOG entry** required — pure refactor, no observable behavior change. (HTTP 507 for genuine 50k+ overflow is technically new, but unreachable on real benchmark data; not worth a CHANGELOG line.) +- **Closure tracker update** after merge: flip §25 Phase 3, 5, 6 status to ✅ in `docs/residual-review-findings/2026-04-24-review-closure-tracker.md`. Include the closing PR number and merge SHA. +- **Follow-up plan candidate**: §25 Phase 7 (`benchmarks/adapters/conversation_mapping.py`) is the next logical refactor PR — closes R2-24, R2-33, R4-P2-8/9. +- **`/ce-compound` candidates** (per learnings researcher): document the Service / Repository / DTO extraction pattern after merge (no existing entry). + +--- + +## Sources & References + +- **Source review**: `.context/compound-engineering/ce-code-review/20260424-152926-6301c860/REVIEW.md` §25 (Design-Pattern Refactor Plan), §25.1 (Behavior-Preserving Refactor table), §25.2 (Abstraction Guardrails), §26.2 Phase 2 (related deferred items). +- **Closure tracker**: `docs/residual-review-findings/2026-04-24-review-closure-tracker.md` (canonical status of every REVIEW.md item). +- **Prior plans**: + - `docs/plans/2026-04-25-003-fix-benchmark-conversation-ingest-review-fixes-plan.md` (landed Phases 1, 2, 4) + - `docs/plans/2026-04-25-004-refactor-benchmark-ingest-p2-residuals-plan.md` (landed P2 polish; introduced `RecompositionEntry` TypedDict) +- **Related PRs (merged)**: #3 (Phase 1), #4 (should-address residuals), #5 (P2 polish), #6 (close defensive + version bump). +- **Branch**: `refactor/server-side-design-patterns` off `master @ 155b218` (post-PR #6 merge). diff --git a/docs/residual-review-findings/2026-04-24-review-closure-tracker.md b/docs/residual-review-findings/2026-04-24-review-closure-tracker.md new file mode 100644 index 0000000..e33b3ed --- /dev/null +++ b/docs/residual-review-findings/2026-04-24-review-closure-tracker.md @@ -0,0 +1,406 @@ +# Closure Tracker — REVIEW.md `20260424-152926-6301c860` + +**Source:** `.context/compound-engineering/ce-code-review/20260424-152926-6301c860/REVIEW.md` +**Original verdict (2026-04-24):** Not ready (P0 + 14 P1) +**Current verdict (2026-04-25):** All Phase 1 verification gates passed; long-tail items tracked below. +**PRs that closed items here:** #3, #4, #5, #6 (all merged into master) + +## Status legend + +| Symbol | Meaning | +|---|---| +| ✅ | Done — closed in a merged PR; commit cited | +| ⚠️ | Partial — some aspect closed, some still open; explained inline | +| ❌ | Not done — open | +| ➖ | Deferred by design — explicitly deferred to follow-up; reason cited | +| 🔄 | N/A — superseded by a later finding or no longer applicable | + +--- + +## Closure summary + +| Bucket | Total | ✅ | ⚠️ | ❌ / ➖ | +|---|---|---|---|---| +| Round 1 findings (§4, F1–F26) | 26 | 18 | 4 | 4 | +| Round 2 P1 (§12, R2-01–R2-09) | 9 | 8 | 1 | 0 | +| Round 2 P2 (§13, R2-10–R2-28) | 19 | 7 | 2 | 10 | +| Round 2 P3 (§14, R2-29–R2-37) | 9 | 1 | 0 | 8 | +| Round 3 store-perf (§20.3, R3-P-*) | 10 | 5 | 0 | 5 | +| Round 3 recall-perf (§20.4, R3-RP-*) | 2 | 0 | 0 | 2 | +| Round 3 recall-correctness (§20.5–6, R3-RC-*) | 9 | 7 | 0 | 2 | +| Round 4 consensus (§22) | 8 | 8 | 0 | 0 | +| Round 4 P2/P3 add-ons (§23) | 13 | 5 | 1 | 7 | +| Pre-existing (§5) | 6 | 2 | 0 | 4 | +| Round 2 Residual Risks (§15) | 7 | 1 | 0 | 6 | +| Round 2 Testing Gaps (§16) | 8 | 1 | 2 | 5 | +| Design-Pattern Refactor (§25, Phases 1–7) | 7 | 3 | 0 | 4 | +| **Totals** | **133** | **66** | **10** | **57** | + +Note: Many items overlap across rounds (e.g., R3-P-12 = R2-01; R2-09 ↔ F6; R2-15 ↔ F13). Each row counts the item once in its earliest-introduced section; overlap notes appear inline. Round 4 mostly confirms prior findings rather than introducing new ones. + +--- + +## Round 1 — Section 4 Findings (F1–F26) + +### P0 — must fix before merge + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| F1 | ✅ | `http/server.py:472` → `http/admin_routes.py` | Admin gate on benchmark endpoint | PR #3 / U1 (`e50810b`) | + +### P1 — strongly recommended before merge + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| F2 | ✅ | `http/models.py:294` | Pydantic payload bounds (segments / messages / content / meta) | PR #3 / U2 (`e50810b`) | +| F3 | ✅ | `context/manager.py:1302` | `except Exception` doesn't catch `CancelledError` | PR #3 / U3 (`4cf2c57`) + autofix F6 (`a2d2188`) for symmetric direct_evidence path | +| F4 | ✅ | `context/manager.py:1214` | Same-session concurrent ingest mixes transcripts | PR #3 / U5 (`ddae6b3`) — transcript hash + 409 | +| F5 | ✅ | `context/manager.py:1302` | No failure-injection test for the rollback branch | PR #3 / U16 (`d8ebcce`) — `tests/test_benchmark_ingest_lifecycle.py` | + +### P2 — state and cleanup + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| F6 | ✅ | `context/manager.py:1302` | `_run_full_session_recomposition` directory URIs not in `cleanup_uris` | PR #4 / REL-02 (`8221bea`) — `RecompositionError` carries created URIs | +| F7 | ⚠️ | `context/manager.py:1214` | `source_uri` written before try, no rollback | Acknowledged by U4/U5 — source intentionally kept for idempotent retry; F5 torn-replay marker handles asymmetry | +| F8 | ✅ | `context/manager.py:1271` | Re-ingest with different `msg_range` orphans old leaves | PR #4 / F5 (`8221bea`) — `_purge_torn_benchmark_run` drops stale leaves under same source | + +### P2 — timeouts + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| F9 | ✅ | `http/server.py:473` | Server-side timeout wrapper | PR #3 / U15 (`e50810b`) — `asyncio.wait_for(540s)` in `admin_routes.py` | + +### P2 — performance + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| F10 | ✅ | `context/manager.py:1286` | Three back-to-back filter scans in response build | PR #3 / R3-P-14 enhancement; reduced to single scan | +| F11 | ➖ | `context/manager.py:1243` | Serial embed for merged leaves; `embed_batch` ignored | **Deferred** — same as R2-08 / R3-P-05 (U12 add_batch). Throughput recovered by U13 cross-conv concurrency. | +| F12 | ✅ | `benchmarks/oc_client.py:239` | `include_session_summary` defaults True; adapter never overrides | PR #3 / U11 (`5ef4583`) — adapter passes False | + +### P2 — code quality + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| F13 | ✅ | `context/manager.py:1302` | `with contextlib.suppress(Exception)` swallows cleanup errors silently | PR #3 / U3 — added `logger.warning(..., exc_info=True)` on per-URI failures | +| F14 | ➖ | `context/manager.py:1168` | `ContextManager` already 3495 lines; benchmark code adds to bloat | **Deferred** — equivalent to §25 Phase 3 (Service Layer). Not done. | +| F15 | ✅ | `context/manager.py:1118` | Recomposition entry shape across 3 sites; introduce TypedDict | PR #5 / U5 (`a39220f`) — `RecompositionEntry` TypedDict | +| F16 | ⚠️ | `context/manager.py:1168` | Naming inconsistency: `benchmark_ingest_conversation` vs `benchmark_conversation_ingest` | Partial — orchestrator facade renamed in U1; ContextManager method still `benchmark_ingest_conversation` (R2-26 / M1 confirms unfixed) | +| F17 | ✅ | `context/manager.py:1174` | Type weakness on `segments: List[List[Dict[str, Any]]]` | PR #3 / U2 — Pydantic `BenchmarkConversationMessage` validates at HTTP boundary | + +### P2 — testing coverage + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| F18 | ⚠️ | `context/manager.py:1278` | `include_session_summary=False` branch untested | Partial — `tests/test_locomo_bench.py` asserts adapter passes False (PR #3); end-to-end no-summary HTTP test still missing (PR #5 review T-007) | +| F19 | ❌ | `context/manager.py:1204` | Empty segments / all-empty-content early return untested | Not done; only logger.info added in PR #3 | +| F20 | ❌ | `tests/test_locomo_bench.py:253` | Adapter store-path test doesn't validate message shape / event_date / time_refs propagation | Not done | +| F21 | ⚠️ | `tests/test_http_server.py:757` | `test_04d` doesn't check per-record `source_uri`, `summary_uri`, `layer_counts` | Partial — `summary_uri` and absence of `layer_counts` now asserted (PR #3); `source_uri` per-record still not checked | +| F22 | ❌ | `context/manager.py:1051` | `_benchmark_segment_meta`, `_benchmark_recomposition_entries`, `_export_memory_record` no direct unit tests | Not done | + +### P3 — optional + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| F23 | ✅ | `context/manager.py:1126` | Segment-level meta merged onto each message entry incorrectly | PR #3 / U9 (`5ef4583`) — dict-merge inverted | +| F24 | ❌ | `context/manager.py:1042` | Names: `_persist_rendered_conversation_source`, `_benchmark_segment_meta`, `_export_memory_record` are vague | Not done | +| F25 | ❌ | `benchmarks/oc_client.py:322` | Document the 600s + `retry_on_timeout=False` contract in docstring | Not done | +| F26 | ⚠️ | `context/manager.py:1170` | `benchmark_ingest_conversation` has no structured logging | Partial — added log on success/idempotent (PR #3+#4); structured timing metrics still missing (= R2-22 / R3-P-09) | + +--- + +## Round 2 — Section 12 P1 (R2-01 – R2-09) + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R2-01 | ✅ | `context/manager.py:1266` | `defer_derive=True` permanently truncates L0/L1 (LoCoMo F1 root cause) | PR #3 / U8 (`f17c7d0`) — schedule + await `_complete_deferred_derive` | +| R2-02 | ✅ | `context/manager.py:1181` | `_session_locks` / `_session_activity` leak per ingest call | PR #3 / U4 (`4cf2c57`) — cleanup tracker; ingest within `async with lock:` releases on exit | +| R2-03 | ✅ | `context/manager.py:1126` | dict-merge order makes `_benchmark_segment_meta` dead code | PR #3 / U9 (`5ef4583`) | +| R2-04 | ✅ | `context/manager.py:1365` | Cross-tenant leak via `layer_counts` in response | PR #3 / U6 (`5ef4583`) — field removed | +| R2-05 | ✅ | `context/manager.py:1214` | Transcript freeze (`_persist_rendered_conversation_source` returns existing without diffing) | PR #3 / U5 + PR #4 / F5 (`8221bea`) — hash mismatch raises 409; torn-replay purges | +| R2-06 | ⚠️ | `context/manager.py:1302` + `orchestrator.py:2898` | Qdrant ↔ CortexFS double-write non-atomic; cleanup asymmetric | Partial — in-memory map sidesteps write-time race for content (U10); per-URI cleanup isolation (REL-04). Underlying double-write atomicity not addressed (no 2PC). | +| R2-07 | ✅ | `benchmarks/adapters/{locomo,conversation}.py` | Cross-conversation serial ingest; R11 unmet | PR #3 / U13 (`f17c7d0`) — `--ingest-concurrency` + `asyncio.Semaphore` + `gather` | +| R2-08 | ➖ | `context/manager.py:1243` + `orchestrator.py` | `CachedEmbedder.embed_batch` exists but new path doesn't use it | **Deferred** — orchestrator-layer `add_batch` change too invasive for the review-fix sequence. Plan 003 §"Deferred to Follow-Up" cites this. | +| R2-09 | ✅ | `context/manager.py:1168` | `_orchestrator.add` internal side-effect failures escape cleanup tracker | PR #4 / REL-02 (`8221bea`) — `RecompositionError` carries partial URIs back to outer tracker | + +--- + +## Round 2 — Section 13 P2 (R2-10 – R2-28) + +### Architecture / Design + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R2-10 | ✅ | `http/server.py:472` → `http/admin_routes.py` | Admin endpoint in business router | PR #3 / U1 (`e50810b`) | +| R2-11 | ❌ | `context/manager.py:1168` | SRP violation: `benchmark_ingest_conversation` packs 6 responsibilities | Not done — = §25 Phase 3 Service Layer | +| R2-12 | ❌ | `context/manager.py:1302` | Cleanup ownership scattered across 4 layers; no transaction boundary | Not done — partially addressed by `_BenchmarkRunCleanup` but not unified | + +### Correctness / data integrity + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R2-13 | ✅ | `_build_anchor_clustered_segments` | Empty-anchor entries bypass `within_caps`, blow LLM context window | PR #3 / U7 (`00f2a1e`) — anchorless branch checks caps; ADV-002 fix (PR #3 patch `059a693`) added seed split | +| R2-14 | ❌ | `context/manager.py:1118` | Cross-input-session merge via shared `msg_index` | Not done — same root as R3-RC-02 | +| R2-15 | ✅ | `context/manager.py:2583` | `_delete_immediate_families` aborts on first failure (orphans rest) | PR #4 / REL-04 (`8221bea`) — per-URI try/except | +| R2-16 | ❌ | `context/manager.py:1250` | `source_uri: ""` empty string semantically undefined | Not done | +| R2-17 | ✅ | `context/manager.py:1268` | Re-ingest different msg_range orphans old merged-leaf URIs | PR #4 / F5 (`8221bea`) — `_purge_torn_benchmark_run` | +| R2-18 | ❌ | `storage/qdrant/adapter.py` | Concurrent same-URI add can produce Qdrant duplicate points (TOCTOU) | Not done — would need point ID via `uuid5(NAMESPACE_URL, uri)` | + +### Performance + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R2-19 | ✅ | `context/manager.py:1286` + `:2135` | Two bit-identical filter scans (refinement of F10) | PR #3 (Phase 1 hydration consolidation) — single scan via `_run_full_session_recomposition` returning records | +| R2-20 | ✅ | `context/manager.py` | `_RECOMPOSE_CLUSTER_MAX_*` = 1_000_000 (effectively no cap) | PR #3 / U7 — reduced to 6_000 / 60 | +| R2-21 | ❌ | `context/manager.py` `_generate_session_summary` | 1-directory case has no short-circuit (still 1 LLM + 2 scans + 1 add) | Not done | +| R2-22 | ❌ | `context/manager.py:1170` | New endpoint zero observability: no `StageTimingCollector`, no metrics | Not done — = R3-P-09; = §25 Phase missing | +| R2-23 | ❌ | `context/manager.py:1041` | `_persist_rendered_conversation_source` redundant `fs.write_context` (orchestrator already schedules) | Not done | + +### Duplication / naming / patterns + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R2-24 | ❌ | `benchmarks/adapters/{conversation,locomo}.py` | 169 lines duplicate (10.83% jscpd); shared helper missing | Not done — = §25 Phase 7 Shared Adapter Helper | +| R2-25 | ❌ | `context/manager.py` | `_export_memory_record` is hand-written projection; `MemorySearchResultItem` exists | Not done | +| R2-26 | ⚠️ | `context/manager.py:1168` | Naming asymmetry: ContextManager method should be `benchmark_conversation_ingest` | Partial — facade + URL renamed (U1); ContextManager method still old name | +| R2-27 | ❌ | `tests/test_locomo_bench.py` `_OCStub` | Hand-maintained test stub vs `AsyncMock(spec_set=OCClient)` | Not done | +| R2-28 | ❌ | `http/models.py:288` | Bare `Dict[str, Any]` response; missing Pydantic envelope | Not done — = §25 Phase 6 DTO | + +--- + +## Round 2 — Section 14 P3 (R2-29 – R2-37) + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R2-29 | ❌ | `context/manager.py:1118` | `immediate_uris: []` / `superseded_merged_uris: []` shape tax in benchmark entries | Not done — TypedDict (U5/PR #5) keeps fields Required; future cleanup candidate | +| R2-30 | ❌ | `context/manager.py:522` | `_export_memory_record` 18-line static helper, single call site, could inline | Not done | +| R2-31 | ✅ | `benchmarks/unified_eval.py:906` | CLI `--ingest-method` help text uses internal term "merged-leaf" | PR #3 (`5ef4583`) — simplified to `store (benchmark-only offline ingest)` | +| R2-32 | ❌ | `context/manager.py:1052` | `speaker` field not aggregated into merged-leaf top-level | Not done | +| R2-33 | ❌ | `benchmarks/adapters/{conversation,locomo}.py` | `turn_id` naming + 0/1-based offset divergence between adapters | Not done | +| R2-34 | ❌ | `context/manager.py` | New helpers (`_benchmark_*`, `_export_*`) deviate from `verb_noun` convention | Not done | +| R2-35 | ❌ | `context/manager.py:1250` | `msg_range` double-encoded (URI + meta) — drift risk | Not done — advisory | +| R2-36 | ❌ | `context/manager.py:1204` | Empty `normalized_segments` returns silently (no warning) | Partial — added `logger.info` (PR #3) but not warning level | +| R2-37 | ❌ | `context/manager.py:2208` | N+1 keywords patch: filter+update when ID known locally (pre-existing) | Not done | + +--- + +## Round 3 — Section 20.3 store-perf (R3-P-*) + +### Mapped to Round 1/2 + +| # | Status | Maps to | Closed by | +|---|---|---|---| +| R3-P-12 | ✅ | = R2-01 | PR #3 / U8 — defer-derive parity (LoCoMo F1 root cause) | +| R3-P-04 | ✅ | = R2-07 | PR #3 / U13 — cross-conv concurrency | +| R3-P-05 | ➖ | = R2-08 | Deferred (U12 add_batch) | +| R3-P-14 | ✅ | = R2-19 / F10 refinement | PR #3 | + +### New in Round 3 + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R3-P-02 | ✅ | `manager.py:2187` | 8 directory `_derive_parent_summary` calls serial (36s); `Semaphore(3)` → 12s | PR #3 / U14 (`00f2a1e`) + PR #3 patch (`059a693`) — instance-scoped semaphore | +| R3-P-01 | ❌ | `manager.py:1183` | Per-session lock held for entire 56s ingest; serializes same-session concurrent requests | Not done | +| R3-P-03 | ❌ | `manager.py:2259` | N+1 keywords patch (overlap with R2-37) | Not done | +| R3-P-06 | ❌ | `orchestrator.py:2510` | `_sync_anchor_projection_records` 2 stale-filter scans on empty defer-derive projection | Not done | +| R3-P-09 | ❌ | `manager.py:1168` | Endpoint zero observability (= R2-22) | Not done | +| R3-P-13 | ➖ | `orchestrator.py:2762` | Local embedder is CPU-bound; `run_in_executor` no concurrency benefit; needs `embed_batch` | Deferred (depends on U12 add_batch) | +| R3-P-07 | ❌ | `cortex_fs.py:1170` | CortexFS write_context 4-worker thread pool bottleneck (advisory P3) | Not done | + +--- + +## Round 3 — Section 20.4 recall-perf (R3-RP-*) + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R3-RP-01 | ➖ | — | Rerank artificially fast on truncated abstracts; quality penalty | Resolved by R3-P-12 / R2-01 (defer-derive parity) — once L0/L1 are LLM-derived, rerank quality recovers. Marker only; no separate fix needed. | +| R3-RP-02 | ❌ | anchor parallel search | Anchor parallel search wastes 4-10ms on near-empty anchor index | Not done — advisory; net effect microscopic | + +--- + +## Round 3 — Section 20.5–6 recall-correctness (R3-RC-*) + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| R3-RC-01 | ✅ | (composite of R2-01 + R2-03 + defer-derive) | Recall systematically degraded on benchmark merged leaves (P1, conf 100) | PR #3 / U8 + U9 (`f17c7d0` + `5ef4583`) — closing R2-01 + R2-03 closes the composite | +| R3-RC-02 | ❌ | `manager.py:1118` | Cross-input-session merge via single `msg_index` stream (= R2-14) | Not done | +| R3-RC-03 | ❌ | `manager.py:1372` | `source_uri=None` filter degrades to session-wide scan | Not done | +| R3-RC-04 | ✅ | `manager.py:1018` | Conversation source freeze (= R2-05 recall-side) | PR #3 / U5 — hash-based versioning | +| R3-RC-05 | ✅ | `manager.py:1126` | dict-merge bug latent until message meta drops time_refs (= R2-03 condition) | PR #3 / U9 — root R2-03 fix covers this | +| R3-RC-06 | ✅ | `manager.py:522` `_export_memory_record` | `content` field always empty | PR #3 / U10 (`5ef4583`) — in-memory hydration | +| R3-RC-07 | ❌ | `_starting_point_probe` | Session summary records may slip into recall candidates | Not done — advisory P3 | +| R3-RC-08 | ❌ | `tests/test_http_server.py` | Test asserts don't include `recomposition_stage='benchmark_offline'` as legal value | Not done | +| R3-RC-09 | ❌ | `manager.py:922` `_merged_leaf_uri` | Adapter relies on URI lex sort = msg_range order; no test pins zero-padding format | Not done | + +--- + +## Round 4 — Section 22 consensus + +Round 4 mostly **confirms** prior P0/P1; entries below already counted in Round 1–3 closure status. + +| Round 4 row | Maps to | Status | +|---|---|---| +| Admin gate missing | F1 / R2-10 | ✅ | +| Request bounds | F2 | ✅ | +| Cancellation rollback | F3 | ✅ | +| Source/payload pollution | F4 / R2-05 | ✅ | +| Summary failure → orphan directories | F6 | ✅ | +| `layer_counts` scope leak | R2-04 | ✅ | +| Anchorless caps | R2-13 | ✅ | +| Benchmark store still serial-dominated | R2-08 / R3-P-05 | ➖ (U12 deferred; offset by U13) | + +--- + +## Round 4 — Section 23 P2/P3 add-ons + +| Row | Status | Title | Closed by | +|---|---|---|---| +| R4-P2-1 | ✅ | Route placement + private member access | PR #3 / U1 + facade | +| R4-P2-2 | ❌ | SRP: extract `BenchmarkConversationIngestService` | Not done — = §25 Phase 3 | +| R4-P2-3 | ❌ | source_uri rollback | Not done — same as F7 acknowledgement | +| R4-P2-4 | ⚠️ | `_load_session_merged_records(source_uri=None)` tenant-isolation footgun | Partial — `source_uri` always set on benchmark path now; underlying helper still unscoped | +| R4-P2-5 | ❌ | `filter(limit=10000)` silent truncation across multiple sites | Not done | +| R4-P2-6 | ❌ | source / directory write goes around orchestrator side-effect boundary (redundant `fs.write_context`) | Not done — = R2-23 | +| R4-P2-7 | ✅ | `_delete_immediate_families` per-URI cleanup isolation | PR #4 / REL-04 (= R2-15) | +| R4-P2-8 | ❌ | adapter store branch duplication; future timeout/summary/batch params will drift | Not done — = §25 Phase 7 / R2-24 | +| R4-P2-9 | ❌ | adapter mapping helper duplication | Not done — = §25 Phase 7 / R2-24 | +| R4-P2-10 | ❌ | Bare `Dict[str, Any]` response; `_export_memory_record` projection | Not done — = R2-28 / §25 Phase 6 | +| R4-P3-1 | ✅ | `args.ingest_method` double state in `unified_eval.py` | PR #3 cleanup | +| R4-P3-2 | ✅ | CLI help "merged-leaf ingest" internal term | PR #3 / R2-31 | +| R4-P3-3 | ❌ | `_OCStub` AsyncMock(spec_set) migration (= R2-27) | Not done | +| R4-P3-4 | ✅ | TypedDict for benchmark entry pseudo-record (= F15) | PR #5 / U5 — `RecompositionEntry` | + +--- + +## Section 5 — Pre-existing (declared "not counted toward verdict but track") + +| # | Status | File:line | Title | Closed by | +|---|---|---|---|---| +| PE-1 | ❌ | `manager.py:2574` | `_delete_immediate_families` mis-named; rename to `_purge_records_and_fs_subtree` | Not done | +| PE-2 | ⚠️ | `manager.py:1378, 1427` | `_load_session_merged_records` / `_session_layer_counts` 10000-row silent truncation | Partial — `_session_layer_counts` no longer in client response (U6); underlying truncation still in helpers | +| PE-3 | ❌ | `manager.py:1181` | `_session_locks` dict unbounded growth; no reaper | Not done | +| PE-4 | ✅ | `manager.py:2208` | `_run_full_session_recomposition` internal serial LLM derive | PR #3 / U14 — `Semaphore(3)` parallelization | +| PE-5 | ✅ | `benchmarks/adapters/{locomo,conversation}.py` | Per-conversation serial ingest in adapter loop | PR #3 / U13 — `asyncio.gather` with semaphore | +| PE-6 | ❌ | `manager.py:1372` | `_load_session_merged_records` filter doesn't include `(tenant, user)` | Not done — partly papered over by always-set `source_uri` (which contains tenant/user) | + +--- + +## Section 15 — Round 2 Residual Risks + +| # | Status | Title | Closed by | +|---|---|---|---| +| RR-1 | ❌ | Observability black box (no metrics on new endpoint) | Not done — = R2-22 / R3-P-09 | +| RR-2 | ❌ | 100k-conversation scale ceiling (single event loop + embedded Qdrant 20GB cap + no checkpoint) | Not done — architectural | +| RR-3 | ❌ | Meta payload validation asymmetry (online uses Pydantic `ToolCallRecord`, benchmark passes through `Dict[str, Any]`) | Not done — U2 added size limits but not structural validation | +| RR-4 | ❌ | Compensation pattern fork: `_BenchmarkRunCleanup` parallel to existing `_restore_merge_snapshot` | Not done | +| RR-5 | ✅ | Anchor-less benchmark sample cluster degeneration (LongMemEval) | PR #3 / U7 — anchorless caps | +| RR-6 | ❌ | URI md5 truncation collision (12 bytes) | Not done — low-risk | +| RR-7 | ❌ | Qdrant `session_id` filter index assumption not pinned in `collection_schemas.py` comment | Not done | + +--- + +## Section 16 — Round 2 Testing Gaps + +| # | Status | Title | Closed by | +|---|---|---|---| +| TG-1 | ⚠️ | Test that benchmark merged leaves get semantic L0/L1 (post-derive quality, not just scheduling) | Partial — PR #3 / U16 asserts derive scheduling; quality assertion needs a real LLM in test fixture (PR #5 review T-001 / T-006 carry this forward) | +| TG-2 | ❌ | Test `_session_locks` cleared after `benchmark_ingest_conversation` returns (= R2-02 lock) | Not done | +| TG-3 | ❌ | Test store-path and mcp-path produce equivalent URI mappings for same fixture | Not done | +| TG-4 | ❌ | Test anchor-less (no time_refs/entities) cluster degeneration (= R2-13 lock) | Not done | +| TG-5 | ⚠️ | Test same-`session_id` re-ingest paths (transcript freeze R2-05, orphan residue R2-17) | Partial — torn-replay covered by PR #4 lifecycle test; transcript content-diff path covered by 409 test; both legs explicit | +| TG-6 | 🔄 | Test `layer_counts` is tenant-isolated (R2-04) | N/A — field deleted in U6 | +| TG-7 | ⚠️ | Test `include_session_summary=False` → no summary record + summary_uri=None | Partial — adapter pass-through asserted; HTTP-level no-summary assert missing (PR #5 review T-007) | +| TG-8 | ❌ | Concurrency tests (same session_id, cross-tenant same session_id, concurrent POST) | Not done | + +--- + +## Section 25 — Design-Pattern Refactor Plan + +| Phase | Pattern | Status | Closed by | +|---|---|---|---| +| 1 | Router Boundary — move benchmark route under admin router | ✅ | PR #3 / U1 | +| 2 | Facade — public `MemoryOrchestrator.benchmark_conversation_ingest` | ✅ | PR #3 / U1 | +| 3 | **Service Layer** — extract `BenchmarkConversationIngestService` (or split helpers) | ❌ | **Not done** | +| 4 | Unit of Work / Cleanup Tracker | ✅ | PR #3 / U4 + PR #4 / REL-02 — `_BenchmarkRunCleanup` + `RecompositionError` | +| 5 | **Repository / Gateway** — wrap session record queries with scope + paging | ❌ | **Not done** | +| 6 | **DTO / Response Model** — `BenchmarkConversationIngestResponse` | ❌ | **Not done** (= R2-28 / R4-P2-10) | +| 7 | **Shared Adapter Helper** — `benchmarks/adapters/conversation_mapping.py` | ❌ | **Not done** (= R2-24 / R4-P2-8/9) | + +### §25.3 Behavior-Changing Decisions Requiring ADR (resolved as part of Phase 1 work) + +| Decision | Resolved as | +|---|---| +| Same-session different transcript | 409 conflict (PR #3 / U5) | +| `include_session_summary` default | Adapter passes False explicitly (PR #3 / U11); endpoint default stays True | +| `layer_counts` response | Field removed (PR #3 / U6) | +| Deferred derive parity | Schedule + await `_complete_deferred_derive` (PR #3 / U8) | +| Cleanup semantics | Source preserved for idempotent retry; merged/directory/summary all in tracker (PR #3 / U4 + PR #4 / REL-02) | + +--- + +## Cross-cutting outside-the-review items also closed in this work + +These were not in REVIEW.md but were discovered and fixed during the closure work: + +| Item | Closed by | +|---|---| +| `MemoryOrchestrator.close()` crashes on partially-initialized instances (`_derive_worker_task` AttributeError in test_perf_fixes) | PR #6 (`f576299`) — defensive `getattr` throughout teardown | +| `pyproject.toml` version 0.6.5 vs `__init__.py` 0.7.0 mismatch | PR #6 (`22468d1`) — bumped pyproject to 0.7.0 | +| Defense-in-depth admin check on `MemoryOrchestrator.benchmark_conversation_ingest` facade | PR #4 / api-contract-007 (`8221bea`) — `enforce_admin=True` default | +| Adapter cross-conv `gather` cancel cascade | PR #4 / REL-04 (`8221bea`) — `return_exceptions=True` | +| Sibling derive task race in U8 gather (orphan FS writes) | PR #3 patch (`059a693`) / F1 must-address — explicit cancel of siblings on first exception | +| Anchorless seed split (single oversized leaf bypassed cap) | PR #3 patch (`059a693`) / ADV-002 — seed-size check | +| Hash transcript list-ordering canonicalization | PR #5 / U6 (`a39220f`) / ADV-006 | + +--- + +## Action queue (the still-open items, ranked) + +### Highest leverage — short, valuable, cumulative + +1. **§25 Phases 3 + 5 + 6** (Service Layer + Repository + DTO) — server-side design refactor. Opens path to fix R2-11, R2-12, R2-22, R2-25, R2-28, R4-P2-2/4/5/6/10, RR-1/4 in one structured pass. +2. **§25 Phase 7** (Shared Adapter Helper) — closes R2-24, R2-33, R4-P2-8/9 in one PR. Orthogonal to §25 server-side; can be parallel. + +### Medium — bug-fix grade, scattered + +3. **R3-RC-02 + R2-14** (cross-input-session merge bug — same root cause, off-by-one risk in URI mapping) +4. **R3-RC-03 + PE-6** (`_load_session_merged_records` tenant filtering) +5. **R2-21** (session_summary 1-directory short-circuit) +6. **R2-23** (redundant `fs.write_context` in `_persist_rendered_conversation_source`) +7. **R3-P-06** (stale-filter short-circuit on empty defer-derive projection) +8. **PE-1** (rename `_delete_immediate_families` → `_purge_records_and_fs_subtree`, update 3 call sites) +9. **PE-3** (`_session_locks` reaper) + +### Long-term residuals — track but don't block + +10. **R2-08 / R3-P-05** (orchestrator `add_batch` + `embed_batch`) — explicitly deferred; only worth picking up after §25 refactor settles +11. **R2-06** (Qdrant ↔ CortexFS write atomicity, no 2PC) — architectural; needs separate ADR +12. **RR-2** (100k-conversation scale ceiling) — architectural; checkpoint/queue-backed ingest +13. **R2-18** (Qdrant TOCTOU duplicate via `uuid5` point ID) — needs schema migration + +### Testing gaps — pair with above where natural + +14. **TG-2** test session_locks teardown — pair with PE-3 +15. **TG-3** test store/mcp URI mapping equivalence — pair with §25 Phase 7 +16. **TG-4** test anchor-less degeneration lock — pair with R2-13 (already fixed in code; test is the lock) +17. **TG-7 + F18 + F19** — `include_session_summary=False`, empty segments, edge cases — small testing PR +18. **TG-8** concurrency tests — pair with R3-P-01 (lock scope reduction) + +### Cosmetic / advisory only + +19. **R2-29 / R2-32 / R2-33 / R2-34 / R2-35 / R2-36 / R2-37** (P3 polish) +20. **R3-P-07** (CortexFS write_context bottleneck) — only matters at higher load +21. **R3-RC-07 / R3-RC-08 / R3-RC-09** (test asserts + URI lex-sort lock) +22. **F24 / F25 / F26** (naming, docstring, structured logging) +23. **RR-6 / RR-7** (md5 collision tracking + `collection_schemas.py` comment) + +--- + +## How to use this tracker + +- **Updating after a PR merges:** flip status, add the closing commit hash. Move the row's "Action queue" entry off the queue. +- **When picking next work:** start from the Action queue's top of the appropriate tier. The Highest-Leverage tier has the best ROI per PR. +- **When a finding gets re-flagged in a future review:** add a row noting the rediscovery; don't delete the original. + +This file is the canonical "what's done from REVIEW.md" record. Keep it next to the source review at `.context/compound-engineering/ce-code-review/20260424-152926-6301c860/REVIEW.md`. diff --git a/src/opencortex/context/manager.py b/src/opencortex/context/manager.py index 20afc70..44198c4 100644 --- a/src/opencortex/context/manager.py +++ b/src/opencortex/context/manager.py @@ -32,6 +32,11 @@ set_request_project_id, ) from opencortex.context.recomposition_types import RecompositionEntry +from opencortex.context.session_records import ( + SessionRecordsRepository, + record_msg_range, + record_text, +) from opencortex.intent import RetrievalPlan, SearchResult from opencortex.intent.retrieval_support import build_probe_scope_input from opencortex.intent.timing import StageTimingCollector, measure_async, measure_sync @@ -257,6 +262,15 @@ def __init__( self._orchestrator = orchestrator self._observer = observer + # Session-scoped record queries (§25 Phase 5 — REVIEW closure + # tracker U1). Constructed once per ContextManager so callers go + # through a single gateway instead of reaching into the storage + # adapter directly. + self._session_records = SessionRecordsRepository( + storage=orchestrator._storage, + collection_resolver=orchestrator._get_collection, + ) + # Prepare cache: {(collection, tid, uid, sid, turn_id): (result, timestamp)} self._prepare_cache: Dict[CacheKey, Tuple[Dict, float]] = {} # Reverse index: {session_key: set(cache_key)} — for end cleanup @@ -1353,7 +1367,7 @@ async def _purge_torn_benchmark_run( if rec.get("uri") ] try: - directory_records = await self._load_session_directory_records( + directory_records = await self._session_records.load_directories( session_id=session_id, source_uri=source_uri, ) @@ -1666,7 +1680,7 @@ async def benchmark_ingest_conversation( # rewriting. Avoids creating duplicate leaves with identical # msg_range URIs and avoids paying recompose / summary costs # again for an already-ingested transcript. - existing_records = await self._load_session_merged_records( + existing_records = await self._session_records.load_merged( session_id=session_id, source_uri=source_uri, ) @@ -1713,10 +1727,8 @@ async def benchmark_ingest_conversation( existing_summary_uri = self._session_summary_uri( tenant_id, user_id, session_id ) - existing_summary = await ( - self._orchestrator._get_record_by_uri( - existing_summary_uri - ) + existing_summary = await self._session_records.load_summary( + existing_summary_uri ) summary_uri_for_response = ( existing_summary_uri if existing_summary else None @@ -1891,7 +1903,7 @@ async def _bounded_complete( source_uri=source_uri, ) - merged_records = await self._load_session_merged_records( + merged_records = await self._session_records.load_merged( session_id=session_id, source_uri=source_uri, ) @@ -2115,113 +2127,17 @@ async def _load_immediate_records( ordered.append(record) return ordered - @staticmethod - def _record_msg_range(record: Dict[str, Any]) -> Optional[Tuple[int, int]]: - """Extract one normalized inclusive ``msg_range`` from a record payload.""" - meta = dict(record.get("meta") or {}) - raw_range = meta.get("msg_range", record.get("msg_range")) - if not isinstance(raw_range, list) or len(raw_range) != 2: - msg_index = meta.get("msg_index") - try: - index = int(msg_index) - except (TypeError, ValueError): - return None - return index, index - try: - start = int(raw_range[0]) - end = int(raw_range[1]) - except (TypeError, ValueError): - return None - if start > end: - return None - return start, end - - @staticmethod - def _record_text(record: Dict[str, Any]) -> str: - """Choose the best available record text for recomposition input.""" - for key in ("content", "overview", "abstract"): - value = str(record.get(key, "") or "").strip() - if value: - return value - return "" - - async def _load_session_merged_records( - self, - *, - session_id: str, - source_uri: Optional[str] = None, - ) -> List[Dict[str, Any]]: - """Load merged conversation leaves for one session in msg-range order.""" - conds: List[Dict[str, Any]] = [ - {"op": "must", "field": "session_id", "conds": [session_id]}, - ] - records = await self._orchestrator._storage.filter( - self._orchestrator._get_collection(), - {"op": "and", "conds": conds}, - limit=10000, - ) - sortable: List[Tuple[int, int, Dict[str, Any]]] = [] - for record in records: - meta = dict(record.get("meta") or {}) - if str(meta.get("layer", "") or "") != "merged": - continue - if source_uri: - if str(meta.get("source_uri", "") or "") != source_uri: - continue - msg_range = self._record_msg_range(record) - if msg_range is None: - continue - sortable.append((msg_range[0], msg_range[1], record)) - sortable.sort(key=lambda item: (item[0], item[1])) - return [record for _, _, record in sortable] - - async def _load_session_directory_records( - self, - *, - session_id: str, - source_uri: Optional[str] = None, - ) -> List[Dict[str, Any]]: - """Load directory parent records for one session in msg-range order.""" - conds: List[Dict[str, Any]] = [ - {"op": "must", "field": "session_id", "conds": [session_id]}, - ] - records = await self._orchestrator._storage.filter( - self._orchestrator._get_collection(), - {"op": "and", "conds": conds}, - limit=10000, - ) - sortable: List[Tuple[int, int, Dict[str, Any]]] = [] - for record in records: - meta = dict(record.get("meta") or {}) - if str(meta.get("layer", "") or "") != "directory": - continue - if source_uri: - if str(meta.get("source_uri", "") or "") != source_uri: - continue - msg_range = self._record_msg_range(record) - if msg_range is None: - continue - sortable.append((msg_range[0], msg_range[1], record)) - sortable.sort(key=lambda item: (item[0], item[1])) - return [record for _, _, record in sortable] - - async def _session_layer_counts(self, session_id: str) -> Dict[str, int]: - """Return per-layer record counts for one session.""" - records = await self._orchestrator._storage.filter( - self._orchestrator._get_collection(), - { - "op": "must", - "field": "session_id", - "conds": [session_id], - }, - limit=10000, - ) - counts: Dict[str, int] = {} - for record in records: - meta = dict(record.get("meta") or {}) - layer = str(meta.get("layer", "") or "") - counts[layer] = counts.get(layer, 0) + 1 - return counts + # NOTE: ``_record_msg_range`` and ``_record_text`` previously lived + # here as staticmethods. They moved to + # ``src/opencortex/context/session_records.py`` as module-level + # ``record_msg_range`` / ``record_text`` (REVIEW §25 Phase 5 / U1) + # so the new ``SessionRecordsRepository`` can reuse them without a + # circular import. ContextManager imports them from there. + # + # ``_load_session_merged_records``, ``_load_session_directory_records``, + # and ``_session_layer_counts`` similarly moved to the repository as + # ``load_merged`` / ``load_directories`` / ``layer_counts``. Call + # sites use ``self._session_records`` instead. async def _select_tail_merged_records( self, @@ -2230,7 +2146,7 @@ async def _select_tail_merged_records( source_uri: str, ) -> List[Dict[str, Any]]: """Select a bounded recent merged-tail window for online recomposition.""" - merged_records = await self._load_session_merged_records( + merged_records = await self._session_records.load_merged( session_id=session_id, source_uri=source_uri, ) @@ -2240,7 +2156,7 @@ async def _select_tail_merged_records( selected: List[Dict[str, Any]] = [] selected_message_count = 0 for record in reversed(merged_records): - msg_range = self._record_msg_range(record) + msg_range = record_msg_range(record) if msg_range is None: continue width = (msg_range[1] - msg_range[0]) + 1 @@ -2407,11 +2323,11 @@ async def _read_l2(uri: str) -> str: l2_by_uri = {} for record in tail_records: - msg_range = self._record_msg_range(record) + msg_range = record_msg_range(record) if msg_range is None: continue uri = str(record.get("uri", "") or "").strip() - text = l2_by_uri.get(uri, "") or self._record_text(record) + text = l2_by_uri.get(uri, "") or record_text(record) if not text: continue entries.append( @@ -2451,7 +2367,7 @@ async def _read_l2(uri: str) -> str: "keywords": "", "entities": [], } - msg_range = self._record_msg_range(record) + msg_range = record_msg_range(record) if msg_range is None: msg_index = snapshot.start_msg_index + offset msg_range = (msg_index, msg_index) @@ -2969,7 +2885,7 @@ async def _run_full_session_recomposition( coll_token = set_collection_name(collection_name) if collection_name else None created_directory_uris: List[str] = [] try: - merged_records = await self._load_session_merged_records( + merged_records = await self._session_records.load_merged( session_id=session_id, source_uri=source_uri, ) @@ -2987,11 +2903,11 @@ async def _run_full_session_recomposition( entries: List[RecompositionEntry] = [] for record in merged_records: - msg_range = self._record_msg_range(record) + msg_range = record_msg_range(record) if msg_range is None: continue uri = str(record.get("uri", "") or "").strip() - text = self._record_text(record) + text = record_text(record) if not text: continue entries.append( @@ -3230,7 +3146,7 @@ async def _generate_session_summary( source_uri: Optional[str], ) -> Optional[str]: """Generate a session-level summary from directory abstracts (or leaf abstracts as fallback).""" - directory_records = await self._load_session_directory_records( + directory_records = await self._session_records.load_directories( session_id=session_id, source_uri=source_uri, ) @@ -3250,7 +3166,7 @@ async def _generate_session_summary( abstracts.append(abstract) # Include ungrouped leaf abstracts (leaves not in any directory) - merged_records = await self._load_session_merged_records( + merged_records = await self._session_records.load_merged( session_id=session_id, source_uri=source_uri, ) @@ -3262,7 +3178,7 @@ async def _generate_session_summary( abstracts.append(abstract) else: # Fallback: use leaf abstracts directly - merged_records = await self._load_session_merged_records( + merged_records = await self._session_records.load_merged( session_id=session_id, source_uri=source_uri, ) @@ -3910,7 +3826,7 @@ def _handle_end_failure( layer_counts: Optional[Dict[str, int]] = None try: - layer_counts = await self._session_layer_counts(session_id) + layer_counts = await self._session_records.layer_counts(session_id) logger.info( "[ContextManager] End state sid=%s source_uri=%s layer_counts=%s", session_id, diff --git a/src/opencortex/context/session_records.py b/src/opencortex/context/session_records.py new file mode 100644 index 0000000..0f85c65 --- /dev/null +++ b/src/opencortex/context/session_records.py @@ -0,0 +1,186 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Session-scoped record queries for the conversation pipeline. + +Wraps the per-session storage filters that ``ContextManager`` previously +exposed as private helpers (``_load_session_merged_records``, +``_load_session_directory_records``, ``_session_layer_counts``, plus the +inline session-summary lookup). Callers go through +``SessionRecordsRepository`` instead of touching the storage adapter +directly so that filter shape, sort discipline, and (eventually) scope ++ paging stay consistent across both the production conversation +lifecycle and the benchmark ingest service. + +This module also re-homes two pure record-reading utilities, +``record_msg_range`` and ``record_text``, that the legacy helpers +relied on. They were previously ``@staticmethod`` on ``ContextManager`` +but have no class state and are needed by the repository's sort logic; +moving them here avoids a circular import. + +REVIEW context for this extraction: +- §25 Phase 5 (Repository / Gateway) of + ``.context/compound-engineering/ce-code-review/20260424-152926-6301c860/REVIEW.md`` +- Closure tracker entries PE-2 (silent 10000-row truncation), R3-RC-03 + (``source_uri=None`` degrades to session-wide scan), PE-6 (cross-tenant + ``session_id`` collision footgun). + +This U1 lands the mechanical extraction with behavior parity. U2 adds +the ``(tenant_id, user_id, source_uri)`` scope discipline + cursor +pagination + overflow guard. +""" + +from __future__ import annotations + +from typing import Any, Callable, Dict, List, Optional, Tuple + + +def record_msg_range(record: Dict[str, Any]) -> Optional[Tuple[int, int]]: + """Extract one normalized inclusive ``msg_range`` from a record payload.""" + meta = dict(record.get("meta") or {}) + raw_range = meta.get("msg_range", record.get("msg_range")) + if not isinstance(raw_range, list) or len(raw_range) != 2: + msg_index = meta.get("msg_index") + try: + index = int(msg_index) + except (TypeError, ValueError): + return None + return index, index + try: + start = int(raw_range[0]) + end = int(raw_range[1]) + except (TypeError, ValueError): + return None + if start > end: + return None + return start, end + + +def record_text(record: Dict[str, Any]) -> str: + """Choose the best available record text for recomposition input.""" + for key in ("content", "overview", "abstract"): + value = str(record.get(key, "") or "").strip() + if value: + return value + return "" + + +class SessionRecordsRepository: + """Read-side gateway for session-scoped record queries. + + Constructed once per ``ContextManager`` instance with a storage + adapter and a callable that resolves the active collection name + (so the repo doesn't capture the whole orchestrator). All methods + are async; storage adapters are async by contract. + + The U1 implementation preserves the legacy helper behavior + bit-for-bit — same filter shape, same in-memory layer-filter, same + msg-range sort. U2 will tighten the filter side to push + ``(tenant_id, user_id, source_uri)`` into the storage filter and + replace the ``limit=10000`` ceiling with cursor-based pagination. + """ + + def __init__( + self, + storage: Any, + collection_resolver: Callable[[], str], + ) -> None: + self._storage = storage + self._collection = collection_resolver + + async def load_merged( + self, + *, + session_id: str, + source_uri: Optional[str] = None, + ) -> List[Dict[str, Any]]: + """Load merged conversation leaves for one session in msg-range order.""" + records = await self._storage.filter( + self._collection(), + { + "op": "and", + "conds": [ + {"op": "must", "field": "session_id", "conds": [session_id]}, + ], + }, + limit=10000, + ) + sortable: List[Tuple[int, int, Dict[str, Any]]] = [] + for record in records: + meta = dict(record.get("meta") or {}) + if str(meta.get("layer", "") or "") != "merged": + continue + if source_uri: + if str(meta.get("source_uri", "") or "") != source_uri: + continue + msg_range = record_msg_range(record) + if msg_range is None: + continue + sortable.append((msg_range[0], msg_range[1], record)) + sortable.sort(key=lambda item: (item[0], item[1])) + return [record for _, _, record in sortable] + + async def load_directories( + self, + *, + session_id: str, + source_uri: Optional[str] = None, + ) -> List[Dict[str, Any]]: + """Load directory parent records for one session in msg-range order.""" + records = await self._storage.filter( + self._collection(), + { + "op": "and", + "conds": [ + {"op": "must", "field": "session_id", "conds": [session_id]}, + ], + }, + limit=10000, + ) + sortable: List[Tuple[int, int, Dict[str, Any]]] = [] + for record in records: + meta = dict(record.get("meta") or {}) + if str(meta.get("layer", "") or "") != "directory": + continue + if source_uri: + if str(meta.get("source_uri", "") or "") != source_uri: + continue + msg_range = record_msg_range(record) + if msg_range is None: + continue + sortable.append((msg_range[0], msg_range[1], record)) + sortable.sort(key=lambda item: (item[0], item[1])) + return [record for _, _, record in sortable] + + async def layer_counts(self, session_id: str) -> Dict[str, int]: + """Return per-layer record counts for one session.""" + records = await self._storage.filter( + self._collection(), + { + "op": "must", + "field": "session_id", + "conds": [session_id], + }, + limit=10000, + ) + counts: Dict[str, int] = {} + for record in records: + meta = dict(record.get("meta") or {}) + layer = str(meta.get("layer", "") or "") + counts[layer] = counts.get(layer, 0) + 1 + return counts + + async def load_summary(self, summary_uri: str) -> Optional[Dict[str, Any]]: + """Fetch the session_summary record at ``summary_uri`` if present. + + Thin wrapper around the orchestrator's URI lookup. The benchmark + idempotent-hit path uses this to decide whether to surface the + prior run's ``summary_uri`` in the response. Returns ``None`` + when no such record exists. + """ + records = await self._storage.filter( + self._collection(), + {"op": "must", "field": "uri", "conds": [summary_uri]}, + limit=1, + ) + if not records: + return None + return records[0] diff --git a/tests/test_session_records_repository.py b/tests/test_session_records_repository.py new file mode 100644 index 0000000..88e7361 --- /dev/null +++ b/tests/test_session_records_repository.py @@ -0,0 +1,213 @@ +"""Behavior parity tests for SessionRecordsRepository. + +Locks the U1 mechanical extraction (see +``docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md``): +the repository methods produce identical results to the legacy +``ContextManager._load_session_*`` / ``_session_layer_counts`` / +session_summary helpers for the same fixtures. U2 will extend this +file with scope + paging + overflow scenarios; U1 only checks parity +on the existing helper shape. +""" + +from __future__ import annotations + +import asyncio +import unittest +from typing import Any, Dict, List + +from opencortex.context.session_records import ( + SessionRecordsRepository, + record_msg_range, + record_text, +) + + +class _FakeStorage: + """Minimal storage stand-in: holds records in a single collection.""" + + def __init__(self, records: List[Dict[str, Any]]) -> None: + self._records = records + + async def filter(self, _collection, where, limit=10000): + # Mirror the in-memory test storage: emit records matching the + # ``session_id`` / ``uri`` conds the legacy helpers used. + targets: List[str] = [] + field = "" + if isinstance(where, dict) and where.get("op") == "and": + for cond in where.get("conds", []) or []: + if cond.get("field") == "session_id": + field = "session_id" + targets = list(cond.get("conds") or []) + break + elif isinstance(where, dict) and where.get("op") == "must": + field = where.get("field", "") + targets = list(where.get("conds") or []) + if not field: + return list(self._records)[:limit] + + out: List[Dict[str, Any]] = [] + for record in self._records: + value = ( + record.get(field) + if field != "uri" + else str(record.get("uri", "") or "") + ) + if field == "session_id": + value = ( + str((record.get("meta") or {}).get("session_id")) + if "meta" in record and record["meta"].get("session_id") + else value + ) + if value in targets: + out.append(record) + if len(out) >= limit: + break + return out + + +def _merged(uri: str, session_id: str, msg_range, source_uri="src"): + return { + "uri": uri, + "session_id": session_id, + "meta": { + "layer": "merged", + "session_id": session_id, + "source_uri": source_uri, + "msg_range": list(msg_range), + }, + } + + +def _directory(uri: str, session_id: str, msg_range, source_uri="src"): + return { + "uri": uri, + "session_id": session_id, + "meta": { + "layer": "directory", + "session_id": session_id, + "source_uri": source_uri, + "msg_range": list(msg_range), + }, + } + + +def _summary(uri: str, session_id: str): + return { + "uri": uri, + "session_id": session_id, + "meta": { + "layer": "session_summary", + "session_id": session_id, + }, + } + + +class TestSessionRecordsRepository(unittest.TestCase): + """Repo-level happy path / edge case parity with legacy helpers.""" + + def _run(self, coro): + return asyncio.run(coro) + + def _repo(self, records): + storage = _FakeStorage(records) + return SessionRecordsRepository( + storage=storage, collection_resolver=lambda: "context" + ) + + def test_load_merged_filters_layer_and_sorts_by_msg_range(self): + """Merged-layer filter + msg_range ascending sort, source_uri scope.""" + + async def check(): + records = [ + _merged("u3", "s1", [4, 5]), + _directory("d1", "s1", [0, 9]), # wrong layer + _merged("u1", "s1", [0, 1]), + _merged("u2", "s1", [2, 3]), + _merged("ux", "s1", [0, 0], source_uri="other"), # other source + _merged("uy", "s2", [0, 1]), # other session + ] + repo = self._repo(records) + out = await repo.load_merged(session_id="s1", source_uri="src") + self.assertEqual([r["uri"] for r in out], ["u1", "u2", "u3"]) + + self._run(check()) + + def test_load_merged_no_source_uri_scope_returns_all_layers_for_session(self): + """source_uri=None preserves legacy behavior — no source filter.""" + + async def check(): + records = [ + _merged("u1", "s1", [0, 1], source_uri="A"), + _merged("u2", "s1", [2, 3], source_uri="B"), + ] + repo = self._repo(records) + out = await repo.load_merged(session_id="s1") + self.assertEqual([r["uri"] for r in out], ["u1", "u2"]) + + self._run(check()) + + def test_load_directories_filters_layer_and_sorts(self): + """Directory-layer filter + msg_range ascending sort.""" + + async def check(): + records = [ + _directory("d2", "s1", [10, 20]), + _merged("u1", "s1", [0, 1]), # wrong layer + _directory("d1", "s1", [0, 9]), + ] + repo = self._repo(records) + out = await repo.load_directories(session_id="s1", source_uri="src") + self.assertEqual([r["uri"] for r in out], ["d1", "d2"]) + + self._run(check()) + + def test_layer_counts_groups_by_layer(self): + """Layer counts dict uses ``meta.layer`` as key.""" + + async def check(): + records = [ + _merged("u1", "s1", [0, 1]), + _merged("u2", "s1", [2, 3]), + _directory("d1", "s1", [0, 1]), + _summary("sum1", "s1"), + ] + repo = self._repo(records) + out = await repo.layer_counts("s1") + self.assertEqual(out, {"merged": 2, "directory": 1, "session_summary": 1}) + + self._run(check()) + + def test_load_summary_returns_record_or_none(self): + """``load_summary`` fetches by URI; returns None when absent.""" + + async def check(): + records = [_summary("opencortex://t/u/session/s1/summary", "s1")] + repo = self._repo(records) + hit = await repo.load_summary("opencortex://t/u/session/s1/summary") + self.assertIsNotNone(hit) + self.assertEqual(hit["uri"], "opencortex://t/u/session/s1/summary") + + miss = await repo.load_summary("opencortex://nonexistent") + self.assertIsNone(miss) + + self._run(check()) + + def test_record_msg_range_extracts_normalized_range(self): + """Pure helper: meta.msg_range → tuple, fallback to msg_index.""" + self.assertEqual(record_msg_range({"meta": {"msg_range": [0, 5]}}), (0, 5)) + self.assertEqual(record_msg_range({"msg_range": [3, 3]}), (3, 3)) + self.assertEqual(record_msg_range({"meta": {"msg_index": 7}}), (7, 7)) + self.assertIsNone(record_msg_range({})) + self.assertIsNone(record_msg_range({"meta": {"msg_range": [5, 0]}})) # inverted + + def test_record_text_prefers_content_then_overview_then_abstract(self): + """Pure helper: text picker order.""" + self.assertEqual(record_text({"content": "c", "overview": "o"}), "c") + self.assertEqual(record_text({"overview": "o", "abstract": "a"}), "o") + self.assertEqual(record_text({"abstract": "a"}), "a") + self.assertEqual(record_text({}), "") + self.assertEqual(record_text({"content": " "}), "") # whitespace stripped + + +if __name__ == "__main__": + unittest.main() From ab6711d709e9c1bb45ebe4e134661c6fff179339 Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 12:39:49 +0800 Subject: [PATCH 2/9] refactor(context): scope discipline + cursor pagination + overflow guard (U2, plan 005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REVIEW §25 Phase 5 second half. SessionRecordsRepository now: - Accepts ``tenant_id`` / ``user_id`` kwargs on ``load_merged``, ``load_directories``, ``layer_counts``. Defaults to ``None`` so U1 behavior is preserved for legacy callers; when provided, the kwargs push ``source_tenant_id`` / ``source_user_id`` conds into the storage filter (closes the cross-tenant ``session_id`` collision footgun PE-6 for callers that have identity available). - Replaces the legacy ``limit=10_000`` silent truncation with cursor-based pagination via ``storage.scroll`` (page_size=1_000, max_pages=50 by default → 50 000 row safety stop). Raises ``SessionRecordOverflowError`` with ``session_id`` + ``count_at_stop`` + ``next_cursor`` + ``method`` when the safety stop fires. - Falls back to a single ``storage.filter`` call with limit = page_size × max_pages when the storage adapter does not implement ``scroll`` (in-memory test fixtures stay supported); the same overflow guard fires when the result set fills the cap. Wired through: - ``benchmark_ingest_conversation`` idempotent-hit and response-build paths now pass ``tenant_id`` + ``user_id`` to ``load_merged``, closing PE-6 on the benchmark hot path. - Admin route maps ``SessionRecordOverflowError`` → HTTP 507 Insufficient Storage with structured detail (reason, session_id, method, count_at_stop, next_cursor, hint). - Production lifecycle callers (``_run_full_session_recomposition``, ``_finalize_session_end``) keep their U1 signatures — threading identity through is a separate cleanup pass. Verification: - 4 new unit tests cover scope kwargs, no-kwargs legacy parity, overflow guard, and scroll pagination loop. All pass. - Full lifecycle regression: 168/169 (sole failure is pre-existing flaky ``test_update_regenerates_fact_points_after_content_change``). --- src/opencortex/context/manager.py | 4 + src/opencortex/context/session_records.py | 222 ++++++++++++++++++---- src/opencortex/http/admin_routes.py | 22 +++ tests/test_session_records_repository.py | 115 +++++++++++ 4 files changed, 331 insertions(+), 32 deletions(-) diff --git a/src/opencortex/context/manager.py b/src/opencortex/context/manager.py index 44198c4..074ab0b 100644 --- a/src/opencortex/context/manager.py +++ b/src/opencortex/context/manager.py @@ -1683,6 +1683,8 @@ async def benchmark_ingest_conversation( existing_records = await self._session_records.load_merged( session_id=session_id, source_uri=source_uri, + tenant_id=tenant_id, + user_id=user_id, ) if existing_records: # Verify the prior run actually completed (REVIEW F5 / @@ -1906,6 +1908,8 @@ async def _bounded_complete( merged_records = await self._session_records.load_merged( session_id=session_id, source_uri=source_uri, + tenant_id=tenant_id, + user_id=user_id, ) # `layer_counts` was previously returned to the client but # the helper that produced it (_session_layer_counts) is diff --git a/src/opencortex/context/session_records.py b/src/opencortex/context/session_records.py index 0f85c65..b06c30a 100644 --- a/src/opencortex/context/session_records.py +++ b/src/opencortex/context/session_records.py @@ -30,8 +30,60 @@ from __future__ import annotations +import logging from typing import Any, Callable, Dict, List, Optional, Tuple +logger = logging.getLogger(__name__) + + +# Default page size when paging through filter results. Picked to keep +# round-trip count low on typical conversation sessions (~50 records) +# while staying well below any single-page memory pressure. +_DEFAULT_PAGE_SIZE = 1_000 + +# Safety stop on the pagination loop. 50 pages × 1 000 rows = 50 000 +# records — orders of magnitude above any realistic single-session +# benchmark or production conversation. Hitting this almost certainly +# means a runaway query (cross-tenant session_id collision in storage, +# session_id payload corruption, or a session that should be rotated). +_DEFAULT_MAX_PAGES = 50 + + +class SessionRecordOverflowError(Exception): + """Raised when a session-scoped query exceeds the safety stop. + + Carries enough context to debug the source: the session_id under + query, the running count when the stop fired, and the next cursor + from the storage adapter (so an operator can resume the scroll + manually if they need the full result set). + + The HTTP admin layer maps this to 507 Insufficient Storage with a + structured detail payload. Production lifecycle callers can catch + it themselves to decide between paging through or surfacing a + diagnostic. + """ + + def __init__( + self, + *, + session_id: str, + count_at_stop: int, + next_cursor: Optional[str], + method: str, + ) -> None: + super().__init__( + f"SessionRecordsRepository.{method}(session_id={session_id!r}) " + f"exceeded the {_DEFAULT_MAX_PAGES}-page safety stop after " + f"{count_at_stop} records (next_cursor={next_cursor!r}). " + "This usually indicates a session_id payload anomaly or a " + "cross-tenant collision; rotate the session_id or audit " + "the storage payload before retrying." + ) + self.session_id = session_id + self.count_at_stop = count_at_stop + self.next_cursor = next_cursor + self.method = method + def record_msg_range(record: Dict[str, Any]) -> Optional[Tuple[int, int]]: """Extract one normalized inclusive ``msg_range`` from a record payload.""" @@ -82,26 +134,132 @@ def __init__( self, storage: Any, collection_resolver: Callable[[], str], + *, + page_size: int = _DEFAULT_PAGE_SIZE, + max_pages: int = _DEFAULT_MAX_PAGES, ) -> None: self._storage = storage self._collection = collection_resolver + self._page_size = page_size + self._max_pages = max_pages + + def _build_session_filter( + self, + *, + session_id: str, + tenant_id: Optional[str] = None, + user_id: Optional[str] = None, + ) -> Dict[str, Any]: + """Compose the storage filter dict for a session-scoped query. + + ``session_id`` is always required. ``tenant_id`` / ``user_id`` + are optional and pushed into the storage filter as additional + ``must`` conditions when provided — this closes the cross-tenant + ``session_id`` collision footgun (REVIEW PE-6) for callers that + have identity available. The relevant indexed payload field + names on the context collection are ``source_tenant_id`` / + ``source_user_id`` (see ``storage/collection_schemas.py``). + + Callers that do not have identity (legacy or maintenance paths) + can still pass ``tenant_id=None`` / ``user_id=None`` to preserve + the U1 behavior — the filter simply omits those conds. + """ + conds: List[Dict[str, Any]] = [ + {"op": "must", "field": "session_id", "conds": [session_id]}, + ] + if tenant_id: + conds.append( + {"op": "must", "field": "source_tenant_id", "conds": [tenant_id]} + ) + if user_id: + conds.append( + {"op": "must", "field": "source_user_id", "conds": [user_id]} + ) + return {"op": "and", "conds": conds} + + async def _scroll_all( + self, + *, + session_id: str, + where: Dict[str, Any], + method: str, + ) -> List[Dict[str, Any]]: + """Page through ``storage.scroll`` with the safety stop guard. + + Replaces the legacy ``limit=10_000`` silent truncation. Loops + until the scroll cursor is exhausted OR ``_max_pages`` pages + have been read; the latter raises ``SessionRecordOverflowError`` + with the cursor + count so the caller can decide whether to + keep paging or surface a diagnostic. Page size is configurable + per repo instance (constructor kwarg). + + Falls back to ``storage.filter`` with ``limit=page_size * + max_pages`` when the storage adapter does not support scroll + (in-memory test fixtures, for example) — this preserves + single-call semantics for those backends while still providing + the overflow signal. + """ + all_records: List[Dict[str, Any]] = [] + scroll = getattr(self._storage, "scroll", None) + if scroll is None or not callable(scroll): + # Fallback for storage adapters without scroll: one filter + # call with a page-size * max-pages limit. Overflow guard + # still fires when the result set hits the cap. + page = await self._storage.filter( + self._collection(), + where, + limit=self._page_size * self._max_pages, + ) + if len(page) >= self._page_size * self._max_pages: + raise SessionRecordOverflowError( + session_id=session_id, + count_at_stop=len(page), + next_cursor=None, + method=method, + ) + return list(page) + + cursor: Optional[str] = None + for page_index in range(self._max_pages): + page, cursor = await scroll( + self._collection(), + filter=where, + limit=self._page_size, + cursor=cursor, + ) + all_records.extend(page) + if cursor is None: + return all_records + # Hit the safety stop — the next cursor is non-None so there + # are more records out there. Surface as overflow. + raise SessionRecordOverflowError( + session_id=session_id, + count_at_stop=len(all_records), + next_cursor=cursor, + method=method, + ) async def load_merged( self, *, session_id: str, source_uri: Optional[str] = None, + tenant_id: Optional[str] = None, + user_id: Optional[str] = None, ) -> List[Dict[str, Any]]: - """Load merged conversation leaves for one session in msg-range order.""" - records = await self._storage.filter( - self._collection(), - { - "op": "and", - "conds": [ - {"op": "must", "field": "session_id", "conds": [session_id]}, - ], - }, - limit=10000, + """Load merged conversation leaves for one session in msg-range order. + + ``tenant_id`` / ``user_id`` (optional) push the cross-tenant + scope into the storage filter — see ``_build_session_filter``. + ``source_uri`` (optional) is filtered in-memory after fetch + because it lives on ``meta.source_uri`` (not a top-level + indexed field on the context collection). + """ + where = self._build_session_filter( + session_id=session_id, tenant_id=tenant_id, user_id=user_id + ) + records = await self._scroll_all( + session_id=session_id, where=where, method="load_merged" ) sortable: List[Tuple[int, int, Dict[str, Any]]] = [] for record in records: @@ -123,17 +281,15 @@ async def load_directories( *, session_id: str, source_uri: Optional[str] = None, + tenant_id: Optional[str] = None, + user_id: Optional[str] = None, ) -> List[Dict[str, Any]]: """Load directory parent records for one session in msg-range order.""" - records = await self._storage.filter( - self._collection(), - { - "op": "and", - "conds": [ - {"op": "must", "field": "session_id", "conds": [session_id]}, - ], - }, - limit=10000, + where = self._build_session_filter( + session_id=session_id, tenant_id=tenant_id, user_id=user_id + ) + records = await self._scroll_all( + session_id=session_id, where=where, method="load_directories" ) sortable: List[Tuple[int, int, Dict[str, Any]]] = [] for record in records: @@ -150,16 +306,19 @@ async def load_directories( sortable.sort(key=lambda item: (item[0], item[1])) return [record for _, _, record in sortable] - async def layer_counts(self, session_id: str) -> Dict[str, int]: + async def layer_counts( + self, + session_id: str, + *, + tenant_id: Optional[str] = None, + user_id: Optional[str] = None, + ) -> Dict[str, int]: """Return per-layer record counts for one session.""" - records = await self._storage.filter( - self._collection(), - { - "op": "must", - "field": "session_id", - "conds": [session_id], - }, - limit=10000, + where = self._build_session_filter( + session_id=session_id, tenant_id=tenant_id, user_id=user_id + ) + records = await self._scroll_all( + session_id=session_id, where=where, method="layer_counts" ) counts: Dict[str, int] = {} for record in records: @@ -171,10 +330,9 @@ async def layer_counts(self, session_id: str) -> Dict[str, int]: async def load_summary(self, summary_uri: str) -> Optional[Dict[str, Any]]: """Fetch the session_summary record at ``summary_uri`` if present. - Thin wrapper around the orchestrator's URI lookup. The benchmark - idempotent-hit path uses this to decide whether to surface the - prior run's ``summary_uri`` in the response. Returns ``None`` - when no such record exists. + Returns ``None`` when no such record exists. URI lookup is + always exactly-one-record, so this method does not paginate + and no overflow guard applies. """ records = await self._storage.filter( self._collection(), diff --git a/src/opencortex/http/admin_routes.py b/src/opencortex/http/admin_routes.py index 20609f3..8d9f189 100644 --- a/src/opencortex/http/admin_routes.py +++ b/src/opencortex/http/admin_routes.py @@ -17,6 +17,7 @@ generate_token, load_token_records, revoke_token, save_token_record, ) from opencortex.context.manager import SourceConflictError +from opencortex.context.session_records import SessionRecordOverflowError from opencortex.http.models import ( BenchmarkConversationIngestRequest, CreateTokenRequest, MemorySearchRequest, RevokeTokenRequest, @@ -291,6 +292,27 @@ async def admin_benchmark_conversation_ingest( "supplied_hash": exc.supplied_hash, }, ) + except SessionRecordOverflowError as exc: + # SessionRecordsRepository safety stop fired — almost certainly a + # session_id payload anomaly or cross-tenant collision in + # storage. Surface 507 with the cursor + count so an operator + # can resume the scroll manually if they actually need the full + # set. (REVIEW closure tracker U2.) + raise HTTPException( + status_code=507, + detail={ + "reason": "session_record_overflow", + "session_id": exc.session_id, + "method": exc.method, + "count_at_stop": exc.count_at_stop, + "next_cursor": exc.next_cursor, + "hint": ( + "Rotate session_id, audit the storage payload for " + "cross-tenant collision, or page manually from " + "next_cursor." + ), + }, + ) # ========================================================================= diff --git a/tests/test_session_records_repository.py b/tests/test_session_records_repository.py index 88e7361..cde1515 100644 --- a/tests/test_session_records_repository.py +++ b/tests/test_session_records_repository.py @@ -16,6 +16,7 @@ from typing import Any, Dict, List from opencortex.context.session_records import ( + SessionRecordOverflowError, SessionRecordsRepository, record_msg_range, record_text, @@ -209,5 +210,119 @@ def test_record_text_prefers_content_then_overview_then_abstract(self): self.assertEqual(record_text({"content": " "}), "") # whitespace stripped +class TestSessionRecordsRepositoryScopeAndOverflow(unittest.TestCase): + """U2: tenant/user scope discipline + cursor pagination + overflow guard.""" + + def _run(self, coro): + return asyncio.run(coro) + + def test_tenant_user_kwargs_push_scope_into_filter(self): + """``load_merged(tenant_id=..., user_id=...)`` adds source_tenant_id/user_id conds.""" + + async def check(): + captured: List[Dict[str, Any]] = [] + + class _CapturingStorage: + async def filter(self, _coll, where, limit=10000): + captured.append(where) + return [] + + repo = SessionRecordsRepository( + storage=_CapturingStorage(), collection_resolver=lambda: "context" + ) + await repo.load_merged( + session_id="s1", tenant_id="t1", user_id="u1" + ) + self.assertEqual(len(captured), 1) + conds = captured[0]["conds"] + fields = {c["field"] for c in conds} + self.assertIn("session_id", fields) + self.assertIn("source_tenant_id", fields) + self.assertIn("source_user_id", fields) + + self._run(check()) + + def test_no_tenant_user_preserves_legacy_filter_shape(self): + """When tenant/user not provided, only session_id cond is pushed.""" + + async def check(): + captured: List[Dict[str, Any]] = [] + + class _CapturingStorage: + async def filter(self, _coll, where, limit=10000): + captured.append(where) + return [] + + repo = SessionRecordsRepository( + storage=_CapturingStorage(), collection_resolver=lambda: "context" + ) + await repo.load_merged(session_id="s1") + self.assertEqual(len(captured), 1) + conds = captured[0]["conds"] + fields = {c["field"] for c in conds} + self.assertEqual(fields, {"session_id"}) + + self._run(check()) + + def test_overflow_raises_session_record_overflow_error(self): + """Filter-fallback path: hitting the page-size * max-pages cap raises.""" + + async def check(): + class _SaturatedStorage: + # No `scroll` attribute → falls through to filter() path. + async def filter(self, _coll, _where, limit=10000): + # Return exactly `limit` records — triggers overflow. + return [ + {"meta": {"layer": "merged", "msg_range": [i, i]}} + for i in range(limit) + ] + + # Tiny budget so the test runs fast. + repo = SessionRecordsRepository( + storage=_SaturatedStorage(), + collection_resolver=lambda: "context", + page_size=10, + max_pages=2, + ) + with self.assertRaises(SessionRecordOverflowError) as ctx: + await repo.load_merged(session_id="huge") + self.assertEqual(ctx.exception.session_id, "huge") + self.assertEqual(ctx.exception.method, "load_merged") + self.assertEqual(ctx.exception.count_at_stop, 20) + + self._run(check()) + + def test_scroll_pagination_loops_until_cursor_none(self): + """Scroll-supporting storage: loop continues until cursor exhausted.""" + + async def check(): + class _ScrollingStorage: + def __init__(self): + # Three pages worth of data. + self._pages = [ + ([{"meta": {"layer": "merged", "msg_range": [0, 0]}}], "c1"), + ([{"meta": {"layer": "merged", "msg_range": [1, 1]}}], "c2"), + ([{"meta": {"layer": "merged", "msg_range": [2, 2]}}], None), + ] + self._index = 0 + + async def scroll(self, _coll, filter=None, limit=10, cursor=None): + page = self._pages[self._index] + self._index += 1 + return page + + repo = SessionRecordsRepository( + storage=_ScrollingStorage(), + collection_resolver=lambda: "context", + page_size=1, + max_pages=10, + ) + out = await repo.load_merged(session_id="s") + # 3 records collected across 3 scroll calls; sorted by msg_range. + self.assertEqual([r["meta"]["msg_range"] for r in out], [[0, 0], [1, 1], [2, 2]]) + + self._run(check()) + + if __name__ == "__main__": unittest.main() From 4f6f001932a20f779d914ace0ce17893204b17cc Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 12:41:56 +0800 Subject: [PATCH 3/9] feat(http): add BenchmarkConversationIngestResponse DTO (U3, plan 005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REVIEW §25 Phase 6. Defines two new Pydantic models in src/opencortex/http/models.py: - ``BenchmarkConversationIngestRecord`` — per-record shape mirroring exactly what ``ContextManager._export_memory_record`` has been returning since PR #3 / U10. Field set is intentionally a superset of what every adapter consumes today; extra fields are optional so the leaner ``direct_evidence`` records validate alongside merged- recompose leaves. The ``content`` field's docstring documents the hydration contract (in-memory map → CortexFS fallback) so future maintainers don't rediscover the R3-RC-06 race. - ``BenchmarkConversationIngestResponse`` — top-level envelope identical to today's dict shape: ``status`` / ``session_id`` / ``source_uri`` / ``summary_uri`` / ``records`` / optional ``ingest_shape``. Replaces the bare ``Dict[str, Any]`` return that R2-28 / R4-P2-10 flagged. The DTOs are added but NOT wired through yet — admin route and service still return dict. U5 wires them. This separation makes U3 trivially safe: zero behavior change, just new types. Verification: - 5 unit tests round-trip representative response shapes from the existing ``test_04d`` (merged_recompose) and ``test_04e`` (direct_evidence) fixtures + edge cases (empty records, minimum required fields, dump round-trip). - No other test suites touched; their dict-based assertions still pass through PR #5 paths unchanged. --- src/opencortex/http/models.py | 89 ++++++++++++ tests/test_benchmark_ingest_response_model.py | 129 ++++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 tests/test_benchmark_ingest_response_model.py diff --git a/src/opencortex/http/models.py b/src/opencortex/http/models.py index 1d37a46..7bdcf67 100644 --- a/src/opencortex/http/models.py +++ b/src/opencortex/http/models.py @@ -365,6 +365,95 @@ class BenchmarkConversationIngestRequest(BaseModel): ) +class BenchmarkConversationIngestRecord(BaseModel): + """One merged-leaf or evidence record returned by benchmark ingest. + + Mirrors the per-record dict shape that ``_export_memory_record`` + has produced since PR #3 / U10. The field set is intentionally a + superset of what every adapter consumes today — extra fields are + optional so ``direct_evidence`` records (which may not carry every + field a ``merged_recompose`` leaf does) still validate. + + REVIEW closure tracker references: R2-28 / R4-P2-10 (typed + response model) and R3-RC-06 (``content`` field hydration — + documented below). + """ + + uri: str = Field(..., description="Stable URI of the stored record.") + abstract: str = Field( + default="", + description="L0 short summary. Post-defer-derive (PR #3 / U8).", + ) + overview: str = Field( + default="", + description="L1 longer overview. Post-defer-derive.", + ) + content: str = Field( + default="", + description=( + "Raw conversation text for this record. Hydrated by the " + "ingest service from the in-memory write-time map (REVIEW " + "R3-RC-06 / U10) so adapters receive the actual segment " + "text rather than what the orchestrator's fire-and-forget " + "CortexFS write may not have flushed yet. Falls back to a " + "CortexFS read for records the in-memory map does not " + "cover (e.g. directory records appended during recompose)." + ), + ) + meta: Dict[str, Any] = Field(default_factory=dict) + abstract_json: Dict[str, Any] = Field(default_factory=dict) + session_id: str = Field(default="") + speaker: str = Field(default="") + event_date: Optional[Any] = Field(default=None) + msg_range: Optional[List[int]] = Field(default=None) + recomposition_stage: Optional[str] = Field(default=None) + source_uri: Optional[str] = Field(default=None) + + +class BenchmarkConversationIngestResponse(BaseModel): + """Typed response envelope for the benchmark conversation ingest endpoint. + + Replaces the bare ``Dict[str, Any]`` return that the admin route + used through PR #5 (REVIEW closure tracker §25 Phase 6 / R2-28 / + R4-P2-10). Field set is intentionally identical to the dict the + benchmark service has been returning — this DTO is a contract + lock, not a contract change. Adapters depending on the existing + JSON shape continue to round-trip cleanly. + + The optional ``ingest_shape`` field is set to ``"direct_evidence"`` + on that path and omitted on ``merged_recompose`` (preserving the + pre-DTO behavior where the merged path's response did not carry + the field). + """ + + status: str = Field( + default="ok", + description="Always ``ok`` on success. Errors raise HTTPException.", + ) + session_id: str + source_uri: Optional[str] = Field( + default=None, + description="Conversation source URI. None when no segments persisted.", + ) + summary_uri: Optional[str] = Field( + default=None, + description=( + "Session summary URI. ``None`` on the ``direct_evidence`` " + "path and on the merged path when ``include_session_summary`` " + "was False. On idempotent-hit replay, surfaces the prior " + "run's summary URI when one was persisted." + ), + ) + records: List[BenchmarkConversationIngestRecord] = Field(default_factory=list) + ingest_shape: Optional[str] = Field( + default=None, + description=( + "Set to ``direct_evidence`` on that ingest path; omitted on " + "the default ``merged_recompose`` path." + ), + ) + + class ContextPrepareIntent(BaseModel): """Intent envelope returned by `/api/v1/context` prepare.""" diff --git a/tests/test_benchmark_ingest_response_model.py b/tests/test_benchmark_ingest_response_model.py new file mode 100644 index 0000000..7cab032 --- /dev/null +++ b/tests/test_benchmark_ingest_response_model.py @@ -0,0 +1,129 @@ +"""DTO validation tests for BenchmarkConversationIngestResponse. + +Locks the U3 contract: the new Pydantic models in +``src/opencortex/http/models.py`` round-trip the existing benchmark +ingest response shape without modification, so wiring the DTO through +the admin route in U5 produces byte-identical JSON. +""" + +from __future__ import annotations + +import unittest + +from opencortex.http.models import ( + BenchmarkConversationIngestRecord, + BenchmarkConversationIngestResponse, +) + + +class TestBenchmarkConversationIngestResponse(unittest.TestCase): + """Round-trip the response shape the service has been returning.""" + + def test_merged_recompose_response_validates(self): + """Representative merged_recompose response dict validates cleanly.""" + payload = { + "status": "ok", + "session_id": "bench_conv_01", + "source_uri": "opencortex://t/u/session/conversations/bench_conv_01/source", + "summary_uri": None, + "records": [ + { + "uri": "opencortex://t/u/memories/events/bench_conv_01-000000-000001", + "abstract": "[Alice] moved to Hangzhou", + "overview": "User Alice notes relocation; Bob notes diet change.", + "content": "[Alice]: I moved to Hangzhou.\n\n[Bob]: You also stopped eating spicy food.", + "meta": { + "layer": "merged", + "msg_range": [0, 1], + "session_id": "bench_conv_01", + "source_uri": "opencortex://t/u/session/conversations/bench_conv_01/source", + "recomposition_stage": "benchmark_offline", + }, + "abstract_json": {"slots": {"entities": ["Alice", "Bob"]}}, + "session_id": "bench_conv_01", + "speaker": "", + "event_date": "2023-05-01T09:00:00Z", + "msg_range": [0, 1], + "recomposition_stage": "benchmark_offline", + "source_uri": "opencortex://t/u/session/conversations/bench_conv_01/source", + } + ], + } + model = BenchmarkConversationIngestResponse.model_validate(payload) + self.assertEqual(model.status, "ok") + self.assertEqual(model.session_id, "bench_conv_01") + self.assertEqual(len(model.records), 1) + self.assertEqual(model.records[0].msg_range, [0, 1]) + self.assertIsNone(model.ingest_shape) + + def test_direct_evidence_response_validates(self): + """direct_evidence path adds ingest_shape; otherwise identical shape.""" + payload = { + "status": "ok", + "session_id": "bench_lme_01", + "source_uri": "opencortex://t/u/session/conversations/bench_lme_01/source", + "summary_uri": None, + "ingest_shape": "direct_evidence", + "records": [ + { + "uri": "opencortex://t/u/memory/events/bench_lme_01/benchmark_evidence_0_0_1", + "content": "user: I moved to Hangzhou.\nassistant: Noted.", + "meta": {"lme_session_id": "s1"}, + "session_id": "bench_lme_01", + "msg_range": [0, 1], + "recomposition_stage": "benchmark_direct_evidence", + } + ], + } + model = BenchmarkConversationIngestResponse.model_validate(payload) + self.assertEqual(model.ingest_shape, "direct_evidence") + self.assertEqual(len(model.records), 1) + + def test_empty_records_list_validates(self): + """Empty segments → empty records list → still validates.""" + payload = { + "status": "ok", + "session_id": "bench_empty", + "source_uri": None, + "summary_uri": None, + "records": [], + } + model = BenchmarkConversationIngestResponse.model_validate(payload) + self.assertEqual(model.records, []) + self.assertIsNone(model.source_uri) + + def test_minimum_required_record_fields(self): + """Record only needs ``uri``; everything else has a sensible default.""" + record = BenchmarkConversationIngestRecord.model_validate( + {"uri": "opencortex://x/y/z"} + ) + self.assertEqual(record.uri, "opencortex://x/y/z") + self.assertEqual(record.abstract, "") + self.assertEqual(record.content, "") + self.assertEqual(record.meta, {}) + self.assertIsNone(record.msg_range) + + def test_response_round_trip_preserves_fields(self): + """``model.model_dump()`` round-trips back to the same field set.""" + payload = { + "status": "ok", + "session_id": "bench_rt", + "source_uri": "opencortex://t/u/session/conversations/bench_rt/source", + "summary_uri": "opencortex://t/u/session/conversations/bench_rt/summary", + "records": [ + { + "uri": "opencortex://t/u/memories/events/bench_rt-000000-000001", + "msg_range": [0, 1], + "recomposition_stage": "benchmark_offline", + } + ], + } + model = BenchmarkConversationIngestResponse.model_validate(payload) + dumped = model.model_dump() + self.assertEqual(dumped["session_id"], "bench_rt") + self.assertEqual(dumped["summary_uri"], payload["summary_uri"]) + self.assertEqual(dumped["records"][0]["msg_range"], [0, 1]) + + +if __name__ == "__main__": + unittest.main() From 39ffd728e232e26946ecc29af1e7c2150cb75c1d Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 12:54:43 +0800 Subject: [PATCH 4/9] refactor(context): extract BenchmarkConversationIngestService (U4, plan 005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §25 Phase 3 — pull the ~390-line benchmark_ingest_conversation body from ContextManager into a dedicated service so each of the six responsibilities lives on its own private method. - New src/opencortex/context/benchmark_ingest_service.py with BenchmarkConversationIngestService(manager, repo). - Public `ingest()` keeps the same dict shape and call signature the admin route already serializes (DTO wiring lands in U5). - Six private methods: _normalize_segments, _idempotent_hit_response, _ingest_merged_recompose, _write_merged_leaves (with sibling-cancel on first exception), _recompose_and_summarize (drains partial directory URIs into cleanup tracker on RecompositionError), _build_response, _ingest_direct_evidence. - ContextManager.benchmark_ingest_conversation becomes a thin shim delegating to self._benchmark_ingest_service.ingest(...) — preserves the existing call path for admin route + tests unchanged. - Service module imports RecompositionError + _BenchmarkRunCleanup lazily inside methods to avoid circular import. - _benchmark_ingest_direct_evidence deleted from manager (service owns it); _benchmark_evidence_uri staticmethod kept (service borrows via manager._benchmark_evidence_uri). Closes REVIEW closure tracker entries: - R2-11 (SRP for benchmark_ingest_conversation) - R2-12 (cleanup ownership boundary clarified — tracker still in manager, but service owns the orchestration) - F14 (ContextManager bloat — manager.py shrinks ~480 lines) Verification: - tests.test_session_records_repository (11 tests) ✅ - tests.test_benchmark_ingest_response_model (5 tests) ✅ - tests.test_e2e_phase1 + tests.test_context_manager — only the pre-existing test_update_regenerates_fact_points_after_content_change failure remains (unrelated to this refactor). - tests.test_http_server + tests.test_locomo_bench + tests.test_benchmark_ingest_lifecycle (51 tests) ✅ Co-Authored-By: Claude Opus 4.7 (1M context) --- .../context/benchmark_ingest_service.py | 687 ++++++++++++++++++ src/opencortex/context/manager.py | 504 +------------ 2 files changed, 712 insertions(+), 479 deletions(-) create mode 100644 src/opencortex/context/benchmark_ingest_service.py diff --git a/src/opencortex/context/benchmark_ingest_service.py b/src/opencortex/context/benchmark_ingest_service.py new file mode 100644 index 0000000..87baff6 --- /dev/null +++ b/src/opencortex/context/benchmark_ingest_service.py @@ -0,0 +1,687 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Benchmark conversation ingest service. + +REVIEW §25 Phase 3 — extracts the orchestration body of +``ContextManager.benchmark_ingest_conversation`` (~390 lines, 6 +responsibilities) into a dedicated service so each responsibility lives +on its own private method. ``ContextManager`` retains the helpers the +service borrows (source persist, recomposition entries build, +recompose, summary generate, hydrate, export, evidence URI build, +session lifecycle dict mutation) — moving those wholesale is a +follow-up. + +The six responsibilities, in order: + +1. ``_normalize_segments`` — strip empty role/content rows; build the + transcript list used by the source-hash check. +2. ``_idempotent_hit_response`` — hash-match short-circuit (returns + the existing records' response when ``run_complete`` is set; + triggers torn-replay purge otherwise). +3. ``_write_merged_leaves`` — per-segment merged-leaf writes plus + ``defer_derive`` task scheduling (with sibling-cancel on first + exception, REVIEW F1 / REL-01 / ADV-001). +4. ``_recompose_and_summarize`` — call recomposition; on failure + drain the partial directory URIs into the cleanup tracker; then + optionally run the summary. +5. ``_build_response`` — load merged records via the repo, hydrate + content, mark source run_complete, return the dict shape the + admin route serializes. +6. ``_ingest_direct_evidence`` — sibling code path for the + ``ingest_shape="direct_evidence"`` branch, kept on the same + service class because it shares the cleanup pattern + run_complete + marker logic. + +The cleanup tracker (``_BenchmarkRunCleanup``) and +``RecompositionError`` continue to live in ``context/manager.py`` — +moving them is a separate cosmetic follow-up. The service imports +both. + +References: +- §25.1 verification gate: behavior golden test (response byte-equal + to legacy method for the same input). +- §25.2 abstraction guardrails: only Service / Repository / DTO; no + Strategy / Abstract Factory. +- Closure tracker entries this unit closes: R2-11 (SRP), R2-12 + (cleanup ownership boundary), F14 (ContextManager bloat). +""" + +from __future__ import annotations + +import asyncio +import contextlib +import logging +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple + +if TYPE_CHECKING: + from opencortex.context.manager import ContextManager + from opencortex.context.session_records import SessionRecordsRepository + +logger = logging.getLogger(__name__) + + +class BenchmarkConversationIngestService: + """Orchestrates the benchmark offline conversation ingest flow. + + Constructed once per ``ContextManager`` (manager owns the + instance and the cleanup tracker / shared semaphores the service + borrows). The service holds a reference to the manager and to the + session records repository — those are the only collaborators it + needs to thread together; everything else lives on one of those + two and is reached via dot-access. + """ + + def __init__( + self, + manager: "ContextManager", + repo: "SessionRecordsRepository", + ) -> None: + self._manager = manager + self._repo = repo + + # ========================================================================= + # Public entry point + # ========================================================================= + + async def ingest( + self, + *, + session_id: str, + tenant_id: str, + user_id: str, + segments: List[List[Dict[str, Any]]], + include_session_summary: bool = True, + ingest_shape: str = "merged_recompose", + ) -> Dict[str, Any]: + """Drive the full benchmark ingest lifecycle for one session. + + Returns the response dict the admin route currently serializes. + U5 will switch this return type to + ``BenchmarkConversationIngestResponse`` (Pydantic). Until then + the dict shape is locked by the existing test suite. + """ + shape = str(ingest_shape or "merged_recompose").strip().lower() + if shape not in {"merged_recompose", "direct_evidence"}: + raise ValueError(f"unsupported benchmark ingest_shape: {ingest_shape}") + + manager = self._manager + sk = manager._make_session_key(tenant_id, user_id, session_id) + manager._touch_session(sk) + manager._remember_session_project(sk) + lock = manager._session_locks.setdefault(sk, asyncio.Lock()) + + async with lock: + normalized_segments, transcript = self._normalize_segments(segments) + + if not normalized_segments: + logger.info( + "benchmark_ingest_conversation: no normalized segments " + "for session %s — returning empty record set", + session_id, + ) + return { + "status": "ok", + "session_id": session_id, + "source_uri": None, + "summary_uri": None, + "records": [], + } + + # SourceConflictError propagates out of the lock — the HTTP + # layer maps it to 409. Raised before the cleanup-tracker + # scope because there is nothing to roll back: the existing + # source belongs to a prior run. + source_uri = await manager._persist_rendered_conversation_source( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + transcript=transcript, + enforce_transcript_hash=True, + ) + + if shape == "direct_evidence": + return await self._ingest_direct_evidence( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + normalized_segments=normalized_segments, + ) + + # Idempotent hit OR torn-replay detection. Returns either + # the cached response (idempotent hit) or None (cold ingest + # after purge / no prior records). + idempotent_response = await self._idempotent_hit_response( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + ) + if idempotent_response is not None: + return idempotent_response + + return await self._ingest_merged_recompose( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + normalized_segments=normalized_segments, + include_session_summary=include_session_summary, + ) + + # ========================================================================= + # Step 1 — normalize incoming segments + # ========================================================================= + + @staticmethod + def _normalize_segments( + segments: List[List[Dict[str, Any]]], + ) -> Tuple[List[List[Dict[str, Any]]], List[Dict[str, Any]]]: + """Strip empty role/content rows; return (segments, transcript). + + ``transcript`` is the flat list used for source-hash computation + in ``_persist_rendered_conversation_source(..., enforce_transcript_hash=True)``. + """ + normalized_segments: List[List[Dict[str, Any]]] = [] + transcript: List[Dict[str, Any]] = [] + for segment in segments: + normalized_messages: List[Dict[str, Any]] = [] + for message in segment: + role = str(message.get("role", "") or "").strip() + content = str(message.get("content", "") or "").strip() + if not role or not content: + continue + normalized = { + "role": role, + "content": content, + "meta": dict(message.get("meta") or {}), + } + normalized_messages.append(normalized) + transcript.append(normalized) + if normalized_messages: + normalized_segments.append(normalized_messages) + return normalized_segments, transcript + + # ========================================================================= + # Step 2 — idempotent hit / torn-replay short-circuit + # ========================================================================= + + async def _idempotent_hit_response( + self, + *, + session_id: str, + tenant_id: str, + user_id: str, + source_uri: str, + ) -> Optional[Dict[str, Any]]: + """Return the cached response if this is a true idempotent replay. + + Returns ``None`` when there are no prior records (cold ingest) OR + when prior records exist but the source's ``run_complete`` marker + is missing (torn prior run — purge inline and fall through to + cold ingest). + """ + manager = self._manager + existing_records = await self._repo.load_merged( + session_id=session_id, + source_uri=source_uri, + tenant_id=tenant_id, + user_id=user_id, + ) + if not existing_records: + return None + + # Verify the prior run actually completed (REVIEW F5 / ADV-007). + # Hash matches but if ``run_complete`` is absent the prior + # ingest crashed before marking itself done — treat as cold + # ingest after purging the stale records. + source_record = await manager._orchestrator._get_record_by_uri(source_uri) + source_meta = ( + dict(source_record.get("meta") or {}) if source_record else {} + ) + run_complete = bool(source_meta.get("run_complete")) + + if not run_complete: + logger.warning( + "benchmark_ingest_conversation: torn prior run " + "detected sid=%s source=%s — purging stale " + "records and re-ingesting", + session_id, + source_uri, + ) + await manager._purge_torn_benchmark_run( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + merged_records=existing_records, + ) + return None + + # Genuine idempotent hit: surface the prior run's summary URI + # if one was persisted (REVIEW F4 / api-contract-005). + existing_summary_uri = manager._session_summary_uri( + tenant_id, user_id, session_id + ) + existing_summary = await self._repo.load_summary(existing_summary_uri) + summary_uri_for_response = ( + existing_summary_uri if existing_summary else None + ) + + logger.info( + "benchmark_ingest_conversation: idempotent hit " + "sid=%s source_uri=%s records=%d summary=%s", + session_id, + source_uri, + len(existing_records), + "present" if existing_summary else "absent", + ) + hydrated = await manager._hydrate_record_contents(existing_records) + return { + "status": "ok", + "session_id": session_id, + "source_uri": source_uri, + "summary_uri": summary_uri_for_response, + "records": [ + manager._export_memory_record( + record, + hydrated_content=hydrated.get( + str(record.get("uri", "") or ""), + "", + ), + ) + for record in existing_records + ], + } + + # ========================================================================= + # Step 3 — merged_recompose ingest path + # ========================================================================= + + async def _ingest_merged_recompose( + self, + *, + session_id: str, + tenant_id: str, + user_id: str, + source_uri: str, + normalized_segments: List[List[Dict[str, Any]]], + include_session_summary: bool, + ) -> Dict[str, Any]: + """Cold-ingest path for ``ingest_shape="merged_recompose"``.""" + from opencortex.context.manager import ( # avoid circular import + RecompositionError, + _BenchmarkRunCleanup, + ) + + manager = self._manager + cleanup = _BenchmarkRunCleanup(source_uri=source_uri) + + try: + merged_content_by_uri = await self._write_merged_leaves( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + normalized_segments=normalized_segments, + cleanup=cleanup, + ) + + if cleanup.merged_uris: + await self._recompose_and_summarize( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + include_session_summary=include_session_summary, + cleanup=cleanup, + ) + + return await self._build_response( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + cleanup=cleanup, + merged_content_by_uri=merged_content_by_uri, + ) + + except asyncio.CancelledError: + # CancelledError descends from BaseException, not Exception, + # so the prior `except Exception` (in the legacy ContextManager + # method) let cancellation (FastAPI request cancel, server + # timeout, client disconnect) bypass cleanup entirely. + # Compensate explicitly, then re-raise so cancellation + # semantics flow through unchanged. + logger.warning( + "benchmark_ingest_conversation cancelled mid-flight " + "sid=%s — running compensation", + session_id, + ) + await cleanup.compensate(manager) + raise + except Exception: + logger.warning( + "benchmark_ingest_conversation failed sid=%s — " + "running compensation", + session_id, + exc_info=True, + ) + await cleanup.compensate(manager) + raise + + # ========================================================================= + # Step 3a — write merged leaves + schedule defer-derive + # ========================================================================= + + async def _write_merged_leaves( + self, + *, + session_id: str, + tenant_id: str, + user_id: str, + source_uri: str, + normalized_segments: List[List[Dict[str, Any]]], + cleanup: Any, + ) -> Dict[str, str]: + """Per-segment merged-leaf writes + bounded-concurrency derive. + + Returns the in-memory map of leaf URI -> raw conversation text + so the response builder can hydrate ``content`` without racing + the orchestrator's fire-and-forget CortexFS write. + """ + manager = self._manager + merged_content_by_uri: Dict[str, str] = {} + derive_tasks: List[Tuple[str, asyncio.Task]] = [] + + async def _bounded_complete( + sem: asyncio.Semaphore, + **dkw: Any, + ) -> None: + async with sem: + await manager._orchestrator._complete_deferred_derive(**dkw) + + entries = manager._benchmark_recomposition_entries(normalized_segments) + offline_segments = manager._build_recomposition_segments(entries) + + for segment in offline_segments: + segment_texts = [str(text) for text in segment.get("messages", [])] + if not segment_texts: + continue + + msg_range = list(segment["msg_range"]) + source_records = segment.get("source_records", []) + merged_meta = await manager._aggregate_records_metadata(source_records) + all_tool_calls: List[Dict[str, Any]] = [] + for record in source_records: + meta = dict(record.get("meta") or {}) + for call in meta.get("tool_calls", []) or []: + if isinstance(call, dict): + all_tool_calls.append(call) + + combined = "\n\n".join(segment_texts) + leaf_meta = { + **merged_meta, + "layer": "merged", + "ingest_mode": "memory", + "msg_range": list(msg_range), + "source_uri": source_uri or "", + "session_id": session_id, + "recomposition_stage": "benchmark_offline", + "tool_calls": all_tool_calls if all_tool_calls else [], + } + merged_context = await manager._orchestrator.add( + uri=manager._merged_leaf_uri( + tenant_id, user_id, session_id, msg_range + ), + abstract="", + content=combined, + category="events", + context_type="memory", + meta=leaf_meta, + session_id=session_id, + dedup=False, + defer_derive=True, + ) + cleanup.merged_uris.append(merged_context.uri) + merged_content_by_uri[merged_context.uri] = combined + + # Schedule LLM derive on the same global semaphore the + # production lifecycle uses. + task = asyncio.create_task( + _bounded_complete( + manager._derive_semaphore, + uri=merged_context.uri, + content=combined, + abstract="", + overview="", + session_id=session_id, + meta=dict(leaf_meta), + raise_on_error=True, + ) + ) + derive_tasks.append((merged_context.uri, task)) + + if derive_tasks: + # Wait for every scheduled derive so the response represents + # post-derive state. Cancel siblings on first exception + # (REVIEW F1 / REL-01 / ADV-001) so they do not race the + # cleanup tracker after compensate runs. + pending = [task for _, task in derive_tasks] + try: + await asyncio.gather(*pending) + except BaseException: + for task in pending: + if not task.done(): + task.cancel() + await asyncio.gather(*pending, return_exceptions=True) + raise + + return merged_content_by_uri + + # ========================================================================= + # Step 3b — recompose + optional summary + # ========================================================================= + + async def _recompose_and_summarize( + self, + *, + session_id: str, + tenant_id: str, + user_id: str, + source_uri: str, + include_session_summary: bool, + cleanup: Any, + ) -> None: + """Run full-session recomposition; optionally generate summary. + + Catches ``RecompositionError`` to drain partial directory URIs + into the cleanup tracker before re-raising the original + exception (REVIEW REL-02). This keeps cleanup ownership in + the run-scoped tracker even when recompose fails partway. + """ + from opencortex.context.manager import ( # avoid circular import + RecompositionError, + ) + + manager = self._manager + try: + directory_uris = await manager._run_full_session_recomposition( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + raise_on_error=True, + return_created_uris=True, + ) + except RecompositionError as exc: + cleanup.directory_uris.extend(exc.created_uris) + raise exc.original from exc + + if directory_uris: + cleanup.directory_uris.extend(directory_uris) + if include_session_summary: + cleanup.summary_uri = await manager._generate_session_summary( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + source_uri=source_uri, + ) + + # ========================================================================= + # Step 3c — build response (load + hydrate + mark run_complete) + # ========================================================================= + + async def _build_response( + self, + *, + session_id: str, + tenant_id: str, + user_id: str, + source_uri: str, + cleanup: Any, + merged_content_by_uri: Dict[str, str], + ) -> Dict[str, Any]: + """Load final merged records, hydrate content, mark run_complete.""" + manager = self._manager + merged_records = await self._repo.load_merged( + session_id=session_id, + source_uri=source_uri, + tenant_id=tenant_id, + user_id=user_id, + ) + # Single-call hydration: in-memory map covers our own writes, + # FS read is the fallback for records that came back from + # ``load_merged`` outside our write set. + hydrated = await manager._hydrate_record_contents( + merged_records, overrides=merged_content_by_uri + ) + # Mark the source as run_complete BEFORE returning so an + # immediate retry sees the marker and short-circuits via the + # idempotent path (REVIEW F5 / ADV-007). + await manager._mark_source_run_complete(source_uri) + return { + "status": "ok", + "session_id": session_id, + "source_uri": source_uri, + "summary_uri": cleanup.summary_uri, + "records": [ + manager._export_memory_record( + record, + hydrated_content=hydrated.get( + str(record.get("uri", "") or ""), + "", + ), + ) + for record in merged_records + ], + } + + # ========================================================================= + # Step 4 — direct_evidence ingest path + # ========================================================================= + + async def _ingest_direct_evidence( + self, + *, + session_id: str, + tenant_id: str, + user_id: str, + source_uri: Optional[str], + normalized_segments: List[List[Dict[str, Any]]], + ) -> Dict[str, Any]: + """Store benchmark segments directly as searchable evidence records. + + Sibling to ``_ingest_merged_recompose``. No recompose, no + directory writes, no session summary; one ``orchestrator.add`` + per input segment. Cleanup uses the legacy + ``_delete_immediate_families`` path because the cleanup tracker + is overkill for this single-write-loop pattern. + """ + manager = self._manager + created_uris: List[str] = [] + records: List[Dict[str, Any]] = [] + evidence_content_by_uri: Dict[str, str] = {} + next_msg_index = 0 + + try: + for segment_index, segment in enumerate(normalized_segments): + segment_texts: List[str] = [] + segment_start = next_msg_index + for message in segment: + meta = dict(message.get("meta") or {}) + role = str(message.get("role", "") or "").strip() + content = str(message.get("content", "") or "").strip() + if not content: + continue + rendered = f"{role}: {content}" if role else content + segment_texts.append( + manager._decorate_message_text(rendered, meta) + ) + next_msg_index += 1 + + if not segment_texts: + continue + + msg_range = [segment_start, next_msg_index - 1] + segment_meta = manager._benchmark_segment_meta(segment) + meta = { + **segment_meta, + "layer": "benchmark_evidence", + "ingest_mode": "memory", + "msg_range": list(msg_range), + "source_uri": source_uri or "", + "session_id": session_id, + "recomposition_stage": "benchmark_direct_evidence", + } + evidence_uri = manager._benchmark_evidence_uri( + tenant_id, user_id, session_id, segment_index, msg_range + ) + combined = "\n".join(segment_texts) + stored = await manager._orchestrator.add( + uri=evidence_uri, + abstract="", + content=combined, + category="events", + context_type="memory", + meta=meta, + session_id=session_id, + dedup=False, + defer_derive=True, + ) + created_uris.append(stored.uri) + evidence_content_by_uri[stored.uri] = combined + record = await manager._orchestrator._get_record_by_uri(stored.uri) + if record: + records.append(record) + + await manager._mark_source_run_complete(source_uri or "") + hydrated = await manager._hydrate_record_contents( + records, overrides=evidence_content_by_uri + ) + return { + "status": "ok", + "session_id": session_id, + "source_uri": source_uri, + "summary_uri": None, + "ingest_shape": "direct_evidence", + "records": [ + manager._export_memory_record( + record, + hydrated_content=hydrated.get( + str(record.get("uri", "") or ""), + "", + ), + ) + for record in records + ], + } + except asyncio.CancelledError: + if created_uris: + with contextlib.suppress(Exception): + await manager._delete_immediate_families(created_uris) + raise + except Exception: + if created_uris: + with contextlib.suppress(Exception): + await manager._delete_immediate_families(created_uris) + raise diff --git a/src/opencortex/context/manager.py b/src/opencortex/context/manager.py index 074ab0b..a3ec949 100644 --- a/src/opencortex/context/manager.py +++ b/src/opencortex/context/manager.py @@ -271,6 +271,17 @@ def __init__( collection_resolver=orchestrator._get_collection, ) + # Benchmark ingest service (§25 Phase 3 — REVIEW closure + # tracker U4). Lazy import avoids the manager <-> service + # circular dependency: the service holds a back-ref to the + # manager and calls many of its private helpers. + from opencortex.context.benchmark_ingest_service import ( + BenchmarkConversationIngestService, + ) + self._benchmark_ingest_service = BenchmarkConversationIngestService( + manager=self, repo=self._session_records + ) + # Prepare cache: {(collection, tid, uid, sid, turn_id): (result, timestamp)} self._prepare_cache: Dict[CacheKey, Tuple[Dict, float]] = {} # Reverse index: {session_key: set(cache_key)} — for end cleanup @@ -1609,372 +1620,22 @@ async def benchmark_ingest_conversation( include_session_summary: bool = True, ingest_shape: str = "merged_recompose", ) -> Dict[str, Any]: - """Benchmark-only offline conversation ingest.""" - shape = str(ingest_shape or "merged_recompose").strip().lower() - if shape not in {"merged_recompose", "direct_evidence"}: - raise ValueError(f"unsupported benchmark ingest_shape: {ingest_shape}") - - sk = self._make_session_key(tenant_id, user_id, session_id) - self._touch_session(sk) - self._remember_session_project(sk) - lock = self._session_locks.setdefault(sk, asyncio.Lock()) - - async with lock: - normalized_segments: List[List[Dict[str, Any]]] = [] - transcript: List[Dict[str, Any]] = [] - - for segment in segments: - normalized_messages: List[Dict[str, Any]] = [] - for message in segment: - role = str(message.get("role", "") or "").strip() - content = str(message.get("content", "") or "").strip() - if not role or not content: - continue - normalized = { - "role": role, - "content": content, - "meta": dict(message.get("meta") or {}), - } - normalized_messages.append(normalized) - transcript.append(normalized) - if normalized_messages: - normalized_segments.append(normalized_messages) - - if not normalized_segments: - logger.info( - "benchmark_ingest_conversation: no normalized segments " - "for session %s — returning empty record set", - session_id, - ) - return { - "status": "ok", - "session_id": session_id, - "source_uri": None, - "summary_uri": None, - "records": [], - } - - # ``SourceConflictError`` propagates out of the lock — the HTTP - # layer maps it to a 409 Conflict. We deliberately raise BEFORE - # entering the cleanup-tracker scope: there is nothing to - # roll back, the existing source belongs to a prior run. - source_uri = await self._persist_rendered_conversation_source( - session_id=session_id, - tenant_id=tenant_id, - user_id=user_id, - transcript=transcript, - enforce_transcript_hash=True, - ) - - if shape == "direct_evidence": - return await self._benchmark_ingest_direct_evidence( - session_id=session_id, - tenant_id=tenant_id, - user_id=user_id, - source_uri=source_uri, - normalized_segments=normalized_segments, - ) - - # Idempotent re-ingest detection: if the source already exists - # AND we have merged leaves under it, return them without - # rewriting. Avoids creating duplicate leaves with identical - # msg_range URIs and avoids paying recompose / summary costs - # again for an already-ingested transcript. - existing_records = await self._session_records.load_merged( - session_id=session_id, - source_uri=source_uri, - tenant_id=tenant_id, - user_id=user_id, - ) - if existing_records: - # Verify the prior run actually completed (REVIEW F5 / - # ADV-007). The transcript hash matches but if the - # ``run_complete`` marker is absent, the prior ingest - # crashed before it could mark itself done — the - # records on disk are a partial / torn set, not a - # genuine idempotent hit. Treat as cold ingest after - # purging the stale records. - source_record = await self._orchestrator._get_record_by_uri( - source_uri - ) - source_meta = ( - dict(source_record.get("meta") or {}) if source_record else {} - ) - run_complete = bool(source_meta.get("run_complete")) - - if not run_complete: - logger.warning( - "benchmark_ingest_conversation: torn prior run " - "detected sid=%s source=%s — purging stale " - "records and re-ingesting", - session_id, - source_uri, - ) - await self._purge_torn_benchmark_run( - session_id=session_id, - tenant_id=tenant_id, - user_id=user_id, - source_uri=source_uri, - merged_records=existing_records, - ) - # fall through to the cold ingest path below - else: - # Look up the prior run's session_summary if one - # exists under the deterministic URI (REVIEW F4 / - # api-contract-005). Previously the idempotent path - # always returned summary_uri=None even when the - # first run had persisted a summary, leaving an - # asymmetric contract that could surprise downstream - # consumers. - existing_summary_uri = self._session_summary_uri( - tenant_id, user_id, session_id - ) - existing_summary = await self._session_records.load_summary( - existing_summary_uri - ) - summary_uri_for_response = ( - existing_summary_uri if existing_summary else None - ) - - logger.info( - "benchmark_ingest_conversation: idempotent hit " - "sid=%s source_uri=%s records=%d summary=%s", - session_id, - source_uri, - len(existing_records), - "present" if existing_summary else "absent", - ) - hydrated = await self._hydrate_record_contents( - existing_records - ) - return { - "status": "ok", - "session_id": session_id, - "source_uri": source_uri, - "summary_uri": summary_uri_for_response, - "records": [ - self._export_memory_record( - record, - hydrated_content=hydrated.get( - str(record.get("uri", "") or ""), - "", - ), - ) - for record in existing_records - ], - } - - cleanup = _BenchmarkRunCleanup(source_uri=source_uri) - # In-memory map URI -> raw conversation text for U10 content - # hydration. Captured as we write each merged leaf so the - # response can return raw text without racing the fire-and- - # forget CortexFS write that ``_orchestrator.add`` schedules. - merged_content_by_uri: Dict[str, str] = {} - # Deferred-derive completion tasks scheduled per leaf (U8). - # Awaited as a batch BEFORE returning so the response carries - # post-derive L0 / L1 / anchors — matching production - # conversation-mode parity. This is what closes the LoCoMo - # F1 0.49 -> 0.33 regression: previously the leaves stayed - # frozen on smart_truncate placeholders forever (R2-01 / - # R3-P-12). - derive_tasks: List[Tuple[str, asyncio.Task]] = [] - - async def _bounded_complete( - sem: asyncio.Semaphore, - **dkw: Any, - ) -> None: - async with sem: - await self._orchestrator._complete_deferred_derive(**dkw) - - try: - entries = self._benchmark_recomposition_entries(normalized_segments) - offline_segments = self._build_recomposition_segments(entries) - - for segment in offline_segments: - segment_texts = [str(text) for text in segment.get("messages", [])] - if not segment_texts: - continue + """Benchmark-only offline conversation ingest. - msg_range = list(segment["msg_range"]) - source_records = segment.get("source_records", []) - merged_meta = await self._aggregate_records_metadata(source_records) - all_tool_calls: List[Dict[str, Any]] = [] - for record in source_records: - meta = dict(record.get("meta") or {}) - for call in meta.get("tool_calls", []) or []: - if isinstance(call, dict): - all_tool_calls.append(call) - - combined = "\n\n".join(segment_texts) - leaf_meta = { - **merged_meta, - "layer": "merged", - "ingest_mode": "memory", - "msg_range": list(msg_range), - "source_uri": source_uri or "", - "session_id": session_id, - "recomposition_stage": "benchmark_offline", - "tool_calls": all_tool_calls if all_tool_calls else [], - } - merged_context = await self._orchestrator.add( - uri=self._merged_leaf_uri( - tenant_id, - user_id, - session_id, - msg_range, - ), - abstract="", - content=combined, - category="events", - context_type="memory", - meta=leaf_meta, - session_id=session_id, - dedup=False, - defer_derive=True, - ) - cleanup.merged_uris.append(merged_context.uri) - merged_content_by_uri[merged_context.uri] = combined - - # Schedule the LLM derive on the same semaphore the - # production lifecycle uses so we never exceed the - # global concurrency budget for merged-leaf derives. - task = asyncio.create_task( - _bounded_complete( - self._derive_semaphore, - uri=merged_context.uri, - content=combined, - abstract="", - overview="", - session_id=session_id, - meta=dict(leaf_meta), - raise_on_error=True, - ) - ) - derive_tasks.append((merged_context.uri, task)) - - if derive_tasks: - # Wait for every scheduled derive so the response - # represents the post-derive Qdrant + CortexFS state. - # - # Plain ``asyncio.gather`` propagates the first exception - # but does not cancel siblings — the surviving tasks - # then call ``_complete_deferred_derive`` against URIs - # that the cleanup tracker has already removed, - # leaving orphan CortexFS subtrees (REVIEW F1 / REL-01 - # / ADV-001). Cancel the siblings explicitly and wait - # for them to unwind before re-raising. - pending = [task for _, task in derive_tasks] - try: - await asyncio.gather(*pending) - except BaseException: - for task in pending: - if not task.done(): - task.cancel() - await asyncio.gather(*pending, return_exceptions=True) - raise - - if cleanup.merged_uris: - try: - directory_uris = ( - await self._run_full_session_recomposition( - session_id=session_id, - tenant_id=tenant_id, - user_id=user_id, - source_uri=source_uri, - raise_on_error=True, - return_created_uris=True, - ) - ) - except RecompositionError as exc: - # Drain partial URIs into the tracker so the - # outer except handler's compensate() removes - # them via the same code path that handles - # successful directory writes (REVIEW REL-02). - # Re-raise the original exception so the outer - # handler still sees the underlying failure - # type (CancelledError, RuntimeError, ...). - cleanup.directory_uris.extend(exc.created_uris) - raise exc.original from exc - if directory_uris: - cleanup.directory_uris.extend(directory_uris) - if include_session_summary: - cleanup.summary_uri = await self._generate_session_summary( - session_id=session_id, - tenant_id=tenant_id, - user_id=user_id, - source_uri=source_uri, - ) + §25 Phase 3 — REVIEW closure tracker U4. The orchestration body + lives in BenchmarkConversationIngestService. This thin shim + preserves the ContextManager-rooted call path so existing + callers (admin route, tests) keep working unchanged. + """ + return await self._benchmark_ingest_service.ingest( + session_id=session_id, + tenant_id=tenant_id, + user_id=user_id, + segments=segments, + include_session_summary=include_session_summary, + ingest_shape=ingest_shape, + ) - merged_records = await self._session_records.load_merged( - session_id=session_id, - source_uri=source_uri, - tenant_id=tenant_id, - user_id=user_id, - ) - # `layer_counts` was previously returned to the client but - # the helper that produced it (_session_layer_counts) is - # session-id-only — not tenant/user/source scoped. Returning - # it allowed any admin caller to enumerate other tenants' - # records by guessing session_ids (R2-04). Adapter doesn't - # consume it; dropping is cheaper than scoping correctly. - # - # Single-call hydration: in-memory map captured during the - # write loop short-circuits the FS read for our own - # leaves, while ``_load_session_merged_records`` may - # surface records (directories, late writes) that need - # the FS fallback. ``_hydrate_record_contents`` returns - # a complete URI -> content map so the comprehension - # below stays a flat lookup. - hydrated = await self._hydrate_record_contents( - merged_records, overrides=merged_content_by_uri - ) - # Mark the source as run_complete BEFORE returning so an - # immediate retry sees the marker and short-circuits via - # the idempotent path (REVIEW F5 / ADV-007). Failure to - # mark is logged but does not fail the response — the - # next replay would just trigger a redundant purge + - # re-ingest, which is correct, just slower. - await self._mark_source_run_complete(source_uri) - return { - "status": "ok", - "session_id": session_id, - "source_uri": source_uri, - "summary_uri": cleanup.summary_uri, - "records": [ - self._export_memory_record( - record, - hydrated_content=hydrated.get( - str(record.get("uri", "") or ""), - "", - ), - ) - for record in merged_records - ], - } - except asyncio.CancelledError: - # CancelledError descends from BaseException, not Exception, - # so the prior `except Exception` let cancellation - # (FastAPI request cancel, server timeout, client disconnect) - # bypass cleanup entirely — leaving orphan merged leaves, - # directory records, and possibly a summary record. Compensate - # explicitly, then re-raise so cancellation semantics flow - # through unchanged. - logger.warning( - "benchmark_ingest_conversation cancelled mid-flight " - "sid=%s — running compensation", - session_id, - ) - await cleanup.compensate(self) - raise - except Exception: - logger.warning( - "benchmark_ingest_conversation failed sid=%s — " - "running compensation", - session_id, - exc_info=True, - ) - await cleanup.compensate(self) - raise @staticmethod def _benchmark_evidence_uri( @@ -1992,121 +1653,6 @@ def _benchmark_evidence_uri( f"benchmark_evidence_{segment_index}_{msg_range[0]}_{msg_range[1]}" ) - async def _benchmark_ingest_direct_evidence( - self, - *, - session_id: str, - tenant_id: str, - user_id: str, - source_uri: Optional[str], - normalized_segments: List[List[Dict[str, Any]]], - ) -> Dict[str, Any]: - """Store benchmark segments directly as searchable evidence records.""" - created_uris: List[str] = [] - records: List[Dict[str, Any]] = [] - # See merged-recompose path: capture combined text per URI so the - # benchmark response can hydrate ``content`` without racing the - # fire-and-forget CortexFS write. - evidence_content_by_uri: Dict[str, str] = {} - next_msg_index = 0 - - try: - for segment_index, segment in enumerate(normalized_segments): - segment_texts: List[str] = [] - segment_start = next_msg_index - for message in segment: - meta = dict(message.get("meta") or {}) - role = str(message.get("role", "") or "").strip() - content = str(message.get("content", "") or "").strip() - if not content: - continue - rendered = f"{role}: {content}" if role else content - segment_texts.append(self._decorate_message_text(rendered, meta)) - next_msg_index += 1 - - if not segment_texts: - continue - - msg_range = [segment_start, next_msg_index - 1] - segment_meta = self._benchmark_segment_meta(segment) - meta = { - **segment_meta, - "layer": "benchmark_evidence", - "ingest_mode": "memory", - "msg_range": list(msg_range), - "source_uri": source_uri or "", - "session_id": session_id, - "recomposition_stage": "benchmark_direct_evidence", - } - evidence_uri = self._benchmark_evidence_uri( - tenant_id, - user_id, - session_id, - segment_index, - msg_range, - ) - combined = "\n".join(segment_texts) - stored = await self._orchestrator.add( - uri=evidence_uri, - abstract="", - content=combined, - category="events", - context_type="memory", - meta=meta, - session_id=session_id, - dedup=False, - defer_derive=True, - ) - created_uris.append(stored.uri) - evidence_content_by_uri[stored.uri] = combined - record = await self._orchestrator._get_record_by_uri(stored.uri) - if record: - records.append(record) - - # Mark source as run_complete so the next replay short-circuits - # via the idempotent path (REVIEW F5 / ADV-007). Symmetric with - # the merged_recompose return path above. - await self._mark_source_run_complete(source_uri or "") - # Single-call hydration via the in-memory write-time map - # (REVIEW KP-06). All evidence URIs are in the override map, - # so this avoids any FS read in the happy path while the - # helper still falls back gracefully if records leak in - # from elsewhere. - hydrated = await self._hydrate_record_contents( - records, overrides=evidence_content_by_uri - ) - return { - "status": "ok", - "session_id": session_id, - "source_uri": source_uri, - "summary_uri": None, - "ingest_shape": "direct_evidence", - "records": [ - self._export_memory_record( - record, - hydrated_content=hydrated.get( - str(record.get("uri", "") or ""), - "", - ), - ) - for record in records - ], - } - except asyncio.CancelledError: - # Symmetric with the merged_recompose path: CancelledError - # descends from BaseException, so the prior `except Exception` - # let cancellation bypass cleanup. Run compensation then - # re-raise so cancellation semantics propagate unchanged. - if created_uris: - with contextlib.suppress(Exception): - await self._delete_immediate_families(created_uris) - raise - except Exception: - if created_uris: - with contextlib.suppress(Exception): - await self._delete_immediate_families(created_uris) - raise - async def _load_immediate_records( self, immediate_uris: List[str], From 85a0ce3add9281ea426f6936dd0b950186de17cd Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 12:58:11 +0800 Subject: [PATCH 5/9] feat(http): wire BenchmarkConversationIngestResponse through admin route (U5, plan 005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §25 Phase 6 wrap-up — declare ``response_model=BenchmarkConversationIngestResponse`` on ``/api/v1/admin/benchmark/conversation_ingest`` and validate the service's dict return through the model before serializing. Locks the endpoint contract: any drift in the dict shape returned by ``BenchmarkConversationIngestService`` (e.g. a future refactor that forgets to populate a record field) surfaces as a 500 here rather than as a silent JSON shape change for downstream adapters. Wire-format note: the merged_recompose path now serializes ``"ingest_shape": null`` instead of omitting the key entirely. All adapters consume ``payload.get("ingest_shape")`` so the change is behavior-neutral. Updated the model docstring to document this. Closes REVIEW closure tracker entries: - R2-28 / R4-P2-10 (typed response model on admin route — replaces the bare ``Dict[str, Any]`` return). Verification: - tests.test_session_records_repository (11) ✅ - tests.test_benchmark_ingest_response_model (5) ✅ - tests.test_http_server (incl. merged + direct_evidence) ✅ - tests.test_locomo_bench (40) ✅ - tests.test_benchmark_ingest_lifecycle ✅ - tests.test_e2e_phase1 + tests.test_context_manager — only the pre-existing test_update_regenerates_fact_points_after_content_change failure remains. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/opencortex/http/admin_routes.py | 19 ++++++++++++++----- src/opencortex/http/models.py | 8 +++++--- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/opencortex/http/admin_routes.py b/src/opencortex/http/admin_routes.py index 8d9f189..b6bd8d6 100644 --- a/src/opencortex/http/admin_routes.py +++ b/src/opencortex/http/admin_routes.py @@ -19,8 +19,8 @@ from opencortex.context.manager import SourceConflictError from opencortex.context.session_records import SessionRecordOverflowError from opencortex.http.models import ( - BenchmarkConversationIngestRequest, CreateTokenRequest, - MemorySearchRequest, RevokeTokenRequest, + BenchmarkConversationIngestRequest, BenchmarkConversationIngestResponse, + CreateTokenRequest, MemorySearchRequest, RevokeTokenRequest, ) from opencortex.http.request_context import ( get_effective_identity, get_effective_role, is_admin, @@ -240,10 +240,13 @@ async def delete_bench_collection(name: str): # Admin — Benchmark (admin-only, benchmark infrastructure) # ========================================================================= -@router.post("/api/v1/admin/benchmark/conversation_ingest") +@router.post( + "/api/v1/admin/benchmark/conversation_ingest", + response_model=BenchmarkConversationIngestResponse, +) async def admin_benchmark_conversation_ingest( req: BenchmarkConversationIngestRequest, -) -> Dict[str, Any]: +) -> BenchmarkConversationIngestResponse: """Benchmark-only offline conversation ingest. This is benchmark infrastructure: it triggers per-leaf embeds, full-session @@ -252,11 +255,16 @@ async def admin_benchmark_conversation_ingest( it is admin-gated and wrapped in a server-side timeout that sits ~10% under the client timeout to ensure the in-process cleanup tracker runs before the client disconnects. + + §25 Phase 6 / U5: response is validated through + ``BenchmarkConversationIngestResponse`` so any drift in the dict + shape returned by ``BenchmarkConversationIngestService`` surfaces + here rather than at adapter parse time. """ _require_admin() tid, uid = get_effective_identity() try: - return await asyncio.wait_for( + result = await asyncio.wait_for( _orchestrator.benchmark_conversation_ingest( session_id=req.session_id, tenant_id=tid, @@ -270,6 +278,7 @@ async def admin_benchmark_conversation_ingest( ), timeout=_BENCHMARK_INGEST_TIMEOUT_SECONDS, ) + return BenchmarkConversationIngestResponse.model_validate(result) except asyncio.TimeoutError: raise HTTPException( status_code=504, diff --git a/src/opencortex/http/models.py b/src/opencortex/http/models.py index 7bdcf67..6d41db3 100644 --- a/src/opencortex/http/models.py +++ b/src/opencortex/http/models.py @@ -421,9 +421,11 @@ class BenchmarkConversationIngestResponse(BaseModel): JSON shape continue to round-trip cleanly. The optional ``ingest_shape`` field is set to ``"direct_evidence"`` - on that path and omitted on ``merged_recompose`` (preserving the - pre-DTO behavior where the merged path's response did not carry - the field). + on that path. On ``merged_recompose`` the service does not set the + field; FastAPI serializes the model attribute as ``null`` in JSON + (a minor wire change from the pre-DTO behavior where the key was + absent entirely — adapters consume ``payload.get("ingest_shape")`` + so neither shape changes their behavior). """ status: str = Field( From 3b0ca91d608b0cab2cc2a01193269911de1cfb05 Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 12:58:36 +0800 Subject: [PATCH 6/9] docs(plan): mark units U1-U5 complete (plan 005) All five implementation units shipped: - U1 SessionRecordsRepository extraction (dded8c9) - U2 scope/paging/overflow guard (ab6711d) - U3 BenchmarkConversationIngestResponse DTO (4f6f001) - U4 BenchmarkConversationIngestService extraction (39ffd72) - U5 DTO wired through admin route (85a0ce3) Co-Authored-By: Claude Opus 4.7 (1M context) --- ...enchmark-ingest-server-side-design-patterns-plan.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md b/docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md index a6dcf9c..114a410 100644 --- a/docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md +++ b/docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md @@ -451,7 +451,7 @@ Key boundary properties: ### Phase 5 — Repository / Gateway -- [ ] U1. **Extract `SessionRecordsRepository` (mechanical move, behavior-preserving)** +- [x] U1. **Extract `SessionRecordsRepository` (mechanical move, behavior-preserving)** **Goal:** Create `src/opencortex/context/session_records.py` housing `SessionRecordsRepository`. Move `_load_session_merged_records`, @@ -498,7 +498,7 @@ constructs a single repo instance once `_orchestrator` is set. --- -- [ ] U2. **Add scope discipline + pagination + overflow guard to Repository** +- [x] U2. **Add scope discipline + pagination + overflow guard to Repository** **Goal:** Each repository method enforces `(tenant_id, user_id, source_uri)` filter discipline — closing PE-6 / R3-RC-03 — and replaces @@ -578,7 +578,7 @@ post-U2 repo must NOT mix them. ### Phase 6 — DTO / Response Model -- [ ] U3. **Define `BenchmarkConversationIngestResponse` Pydantic models** +- [x] U3. **Define `BenchmarkConversationIngestResponse` Pydantic models** **Goal:** Add `BenchmarkConversationIngestResponse` and `BenchmarkConversationIngestRecord` to `src/opencortex/http/models.py`. @@ -640,7 +640,7 @@ relative to them, but easiest after U1 to avoid context-switching). ### Phase 3 — Service Layer -- [ ] U4. **Extract `BenchmarkConversationIngestService`** +- [x] U4. **Extract `BenchmarkConversationIngestService`** **Goal:** Pull the body of `ContextManager.benchmark_ingest_conversation` (391 lines) and `_benchmark_ingest_direct_evidence` into a new @@ -774,7 +774,7 @@ byte-equal response dicts. Land that test, then refactor. ### Phase 6 (continued) — Wire DTO through the public path -- [ ] U5. **Wire `BenchmarkConversationIngestResponse` through Service + admin route** +- [x] U5. **Wire `BenchmarkConversationIngestResponse` through Service + admin route** **Goal:** `Service.ingest` returns `BenchmarkConversationIngestResponse` instead of `Dict[str, Any]`. Admin route declares the DTO as the From 3e249715d72e0f32c120383f7225510c589712e0 Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 13:14:43 +0800 Subject: [PATCH 7/9] fix(review): apply autofix feedback (plan 005, run 20260425-130006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CE review autofix pass — 8 deterministic safe_auto findings applied. Residual gated_auto / manual / advisory findings are surfaced in the PR body for downstream resolution. Mechanical fixes: - KP-01 (P1): BenchmarkConversationIngestRecord.event_date typed Optional[Any] → Optional[str]. Restores DTO validation for the field; production data is always ISO-8601 string or None. - KP-02 (P1): models.py adds `from __future__ import annotations` for consistency with the two new context/ modules. - KP-03 (P1): _write_merged_leaves / _recompose_and_summarize / _build_response cleanup parameter typed `Any` → `_BenchmarkRunCleanup` via TYPE_CHECKING import. Surface the contract to type-checkers. - KP-04 (P2): SessionRecordsRepository.__init__ storage parameter typed `Any` → `StorageInterface` via TYPE_CHECKING import. - KP-06 (P2): unused page_index loop variable → `_`. - KP-09 (P3): _ingest_direct_evidence dedupes the identical CancelledError + Exception handlers into a single `except BaseException`. Also catches a second cancellation that the prior `except Exception` would have leaked. - correctness-002 / ADV-U-002 (P2): _scroll_all fallback overflow guard requested `limit = page_size * max_pages` and raised on `len >= limit`, false-positive at exact-cap boundaries. Now requests `cap + 1` and raises only on `len > cap`. Test updated to assert the new count_at_stop = cap + 1 semantics. - PS-001 (P2): CLAUDE.md context/ directory map adds the three new modules (session_records, benchmark_ingest_service, recomposition_types). Also fixes the pre-existing stale reference storage/vikingdb_interface.py → storage/storage_interface.py. Verification: - tests.test_session_records_repository (11) ✅ (1 test updated) - tests.test_benchmark_ingest_response_model (5) ✅ - tests.test_http_server / test_locomo_bench / test_benchmark_ingest_lifecycle ✅ - tests.test_e2e_phase1 + tests.test_context_manager — only the pre-existing test_update_regenerates_fact_points_after_content_change failure remains. Co-Authored-By: Claude Opus 4.7 (1M context) --- CLAUDE.md | 5 +++- .../context/benchmark_ingest_service.py | 24 +++++++++++-------- src/opencortex/context/session_records.py | 21 ++++++++++------ src/opencortex/http/models.py | 4 +++- tests/test_session_records_repository.py | 7 +++++- 5 files changed, 41 insertions(+), 20 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9db25b0..a71a143 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -44,8 +44,11 @@ src/opencortex/ __main__.py # CLI entry point (opencortex-server) context/ manager.py # ContextManager — Memory Context Protocol lifecycle + session_records.py # SessionRecordsRepository — session-scoped record queries (paging + scope) + benchmark_ingest_service.py # BenchmarkConversationIngestService — admin benchmark ingest orchestration + recomposition_types.py # RecompositionError + shared recomposition dataclasses storage/ - vikingdb_interface.py # Abstract interface (25 async methods) + storage_interface.py # Abstract interface (25 async methods) cortex_fs.py # CortexFS three-layer filesystem (formerly VikingFS) collection_schemas.py # Collection schemas (includes reward scoring fields) qdrant/ diff --git a/src/opencortex/context/benchmark_ingest_service.py b/src/opencortex/context/benchmark_ingest_service.py index 87baff6..12eea51 100644 --- a/src/opencortex/context/benchmark_ingest_service.py +++ b/src/opencortex/context/benchmark_ingest_service.py @@ -53,7 +53,7 @@ from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple if TYPE_CHECKING: - from opencortex.context.manager import ContextManager + from opencortex.context.manager import ContextManager, _BenchmarkRunCleanup from opencortex.context.session_records import SessionRecordsRepository logger = logging.getLogger(__name__) @@ -381,7 +381,7 @@ async def _write_merged_leaves( user_id: str, source_uri: str, normalized_segments: List[List[Dict[str, Any]]], - cleanup: Any, + cleanup: "_BenchmarkRunCleanup", ) -> Dict[str, str]: """Per-segment merged-leaf writes + bounded-concurrency derive. @@ -490,7 +490,7 @@ async def _recompose_and_summarize( user_id: str, source_uri: str, include_session_summary: bool, - cleanup: Any, + cleanup: "_BenchmarkRunCleanup", ) -> None: """Run full-session recomposition; optionally generate summary. @@ -538,7 +538,7 @@ async def _build_response( tenant_id: str, user_id: str, source_uri: str, - cleanup: Any, + cleanup: "_BenchmarkRunCleanup", merged_content_by_uri: Dict[str, str], ) -> Dict[str, Any]: """Load final merged records, hydrate content, mark run_complete.""" @@ -675,12 +675,16 @@ async def _ingest_direct_evidence( for record in records ], } - except asyncio.CancelledError: - if created_uris: - with contextlib.suppress(Exception): - await manager._delete_immediate_families(created_uris) - raise - except Exception: + except BaseException: + # Single handler covers both ``asyncio.CancelledError`` (a + # ``BaseException`` since Python 3.8) and ``Exception``: the + # cleanup body is identical and the bare ``raise`` re-raises + # the active exception unchanged. Catching ``BaseException`` + # also ensures cleanup runs on a second cancellation, where + # ``except Exception`` would let the ingest leak created + # URIs (REVIEW KP-09 / closely related to REL-03 — kept as + # ``Exception`` suppress in the cleanup body so a transient + # storage error during cleanup does not mask the original). if created_uris: with contextlib.suppress(Exception): await manager._delete_immediate_families(created_uris) diff --git a/src/opencortex/context/session_records.py b/src/opencortex/context/session_records.py index b06c30a..6da121e 100644 --- a/src/opencortex/context/session_records.py +++ b/src/opencortex/context/session_records.py @@ -31,7 +31,10 @@ from __future__ import annotations import logging -from typing import Any, Callable, Dict, List, Optional, Tuple +from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Tuple + +if TYPE_CHECKING: + from opencortex.storage.storage_interface import StorageInterface logger = logging.getLogger(__name__) @@ -132,7 +135,7 @@ class SessionRecordsRepository: def __init__( self, - storage: Any, + storage: "StorageInterface", collection_resolver: Callable[[], str], *, page_size: int = _DEFAULT_PAGE_SIZE, @@ -203,14 +206,18 @@ async def _scroll_all( scroll = getattr(self._storage, "scroll", None) if scroll is None or not callable(scroll): # Fallback for storage adapters without scroll: one filter - # call with a page-size * max-pages limit. Overflow guard - # still fires when the result set hits the cap. + # call with a page-size * max-pages limit. Request limit+1 + # so the overflow guard fires only when the result set + # genuinely exceeds the cap (REVIEW correctness-002 / + # ADV-U-002 — equality at the cap was a false-positive + # boundary). + cap = self._page_size * self._max_pages page = await self._storage.filter( self._collection(), where, - limit=self._page_size * self._max_pages, + limit=cap + 1, ) - if len(page) >= self._page_size * self._max_pages: + if len(page) > cap: raise SessionRecordOverflowError( session_id=session_id, count_at_stop=len(page), @@ -220,7 +227,7 @@ async def _scroll_all( return list(page) cursor: Optional[str] = None - for page_index in range(self._max_pages): + for _ in range(self._max_pages): page, cursor = await scroll( self._collection(), filter=where, diff --git a/src/opencortex/http/models.py b/src/opencortex/http/models.py index 6d41db3..b0a7e86 100644 --- a/src/opencortex/http/models.py +++ b/src/opencortex/http/models.py @@ -5,6 +5,8 @@ ``mcp_server.py``. """ +from __future__ import annotations + from typing import Any, Dict, List, Optional import orjson @@ -404,7 +406,7 @@ class BenchmarkConversationIngestRecord(BaseModel): abstract_json: Dict[str, Any] = Field(default_factory=dict) session_id: str = Field(default="") speaker: str = Field(default="") - event_date: Optional[Any] = Field(default=None) + event_date: Optional[str] = Field(default=None) msg_range: Optional[List[int]] = Field(default=None) recomposition_stage: Optional[str] = Field(default=None) source_uri: Optional[str] = Field(default=None) diff --git a/tests/test_session_records_repository.py b/tests/test_session_records_repository.py index cde1515..2c5f79f 100644 --- a/tests/test_session_records_repository.py +++ b/tests/test_session_records_repository.py @@ -288,7 +288,12 @@ async def filter(self, _coll, _where, limit=10000): await repo.load_merged(session_id="huge") self.assertEqual(ctx.exception.session_id, "huge") self.assertEqual(ctx.exception.method, "load_merged") - self.assertEqual(ctx.exception.count_at_stop, 20) + # cap = page_size * max_pages = 20. Repo requests cap+1 = 21 + # records and only raises when len(page) > cap, so the + # storage returns 21 and count_at_stop reflects that. + # REVIEW correctness-002 / ADV-U-002: the legacy ``>=`` check + # raised at exactly cap, a false-positive boundary case. + self.assertEqual(ctx.exception.count_at_stop, 21) self._run(check()) From d83cc534983028664675a1ac3e94e9d1e8ccba11 Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 13:18:13 +0800 Subject: [PATCH 8/9] docs(review): record residual review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Captures gated_auto / manual / advisory residuals from CE autofix run 20260425-130006-07250c15 against plan 005. 8 safe_auto fixes were already applied in commit 3e24971. The residuals here need human judgment before landing — most are P1/P2 spread across reliability, testing, and performance. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../refactor-server-side-design-patterns.md | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 docs/residual-review-findings/refactor-server-side-design-patterns.md diff --git a/docs/residual-review-findings/refactor-server-side-design-patterns.md b/docs/residual-review-findings/refactor-server-side-design-patterns.md new file mode 100644 index 0000000..7c775dd --- /dev/null +++ b/docs/residual-review-findings/refactor-server-side-design-patterns.md @@ -0,0 +1,75 @@ +# Residual Review Findings — refactor/server-side-design-patterns + +**Run:** `.context/compound-engineering/ce-code-review/20260425-130006-07250c15/` +**Branch:** `refactor/server-side-design-patterns` +**HEAD:** `3e24971` +**Plan:** [docs/plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md](../plans/2026-04-25-005-refactor-benchmark-ingest-server-side-design-patterns-plan.md) +**CE Run summary:** [SUMMARY.md](../../.context/compound-engineering/ce-code-review/20260425-130006-07250c15/SUMMARY.md) + +This file records residual `gated_auto` / `manual` findings from the CE +autofix review of plan 005 (Service + Repository + DTO refactor). +8 `safe_auto` findings were already applied in commit `3e24971`. The +findings below are the durable record of work that needs human +judgment before landing. + +## Residual Review Findings + +### P1 (8 findings) + +- **AC-01** [gated_auto] `src/opencortex/http/models.py:447` — Wire-format change: `merged_recompose` path now serializes `"ingest_shape": null` instead of omitting the key. All known Python adapters use `payload.get(...)` so unaffected. Decision: ship as-is or add `model_config = ConfigDict(...)` with `exclude_none=True` to restore pre-DTO wire shape if strict-schema clients exist. (api-contract, confidence 80) +- **AC-02** [gated_auto] `benchmarks/oc_client.py:21` — `OCClient` retries all `status >= 500` responses 8 times with backoff. New 507 from `SessionRecordOverflowError` is deterministic; retries waste ~4 minutes per affected session. Fix: exclude 507 from retryable set. (api-contract, confidence 85) +- **REL-01** [manual] `src/opencortex/context/manager.py:1380-1386` — `_purge_torn_benchmark_run` calls `load_directories()` inside a generic `except Exception` that silently swallows `SessionRecordOverflowError`. Directory-layer records from a failed prior run become permanent storage orphans with no cleanup signal. (reliability, confidence 90) +- **correctness-001** [manual] `src/opencortex/context/benchmark_ingest_service.py:271-274` — Idempotent-hit summary lookup migrated from `_get_record_by_uri` (catches Exception, returns None) to `_repo.load_summary` (raises). Transient storage error during summary lookup now fails the entire idempotent path instead of degrading to `summary_uri=None`. (correctness, confidence 75) +- **T-01** [manual] `src/opencortex/context/benchmark_ingest_service.py` — No direct unit tests for `BenchmarkConversationIngestService`. The `ValueError` on unsupported `ingest_shape` (line 110) and the empty-segments early-exit (lines 127-133) are completely untested. Add `tests/test_benchmark_ingest_service.py`. (testing, confidence 90) +- **T-02** [manual] `src/opencortex/context/benchmark_ingest_service.py:470-483` — Sibling-cancel branch in `_write_merged_leaves` is unreachable by current tests. Existing failure injection at `test_benchmark_ingest_lifecycle.py:323` patches `_run_full_session_recomposition` which fires AFTER all derive tasks have completed. Inject failure into `_complete_deferred_derive` for one leaf while others are in-flight. (testing, confidence 88) +- **T-03** [manual] `src/opencortex/context/benchmark_ingest_service.py:516-518` — `RecompositionError` drain branch unreachable; existing test raises `RuntimeError` directly, bypassing the `RecompositionError` wrapper. Add a test that raises `RecompositionError(original=..., created_uris=[...])` and asserts the partial directory URIs are routed through cleanup. (testing, confidence 85) +- **M-01** [gated_auto] `src/opencortex/context/manager.py:1613-1637` — `benchmark_ingest_conversation` is a 7-line pure pass-through shim. Decision: inline into orchestrator facade (one fewer hop) or schedule for explicit removal. (maintainability, confidence 80) +- **ADV-U-001** [manual] `src/opencortex/context/manager.py:1381` — `_purge_torn_benchmark_run.load_directories()` does NOT pass tenant/user scope (every other repo call site does). Combined with `_delete_immediate_families` consuming the URI list directly, on torn-replay this could hard-delete another tenant's directory subtree if `meta.source_uri` collides as a string. Pair with REL-01. (adversarial, confidence 65) + +### P2 (12 findings) + +- **T-04** [manual] `src/opencortex/context/session_records.py:233-240` — Scroll-path overflow guard untested; only filter-fallback path covered. Add a `_ScrollingStorage` that always returns a non-None cursor. (testing, confidence 82) +- **T-05** [manual] Idempotent-hit + existing summary path untested. Every test hardcodes `include_session_summary=False`; load_summary never returns a record. (testing, confidence 80) +- **T-06** [manual] `src/opencortex/http/admin_routes.py:307-327` — HTTP 507 handler has zero test coverage. Add an integration test that injects a saturated scroll storage and asserts status_code=507 with structured detail. (testing, confidence 78) +- **M-02** [manual] `BenchmarkConversationIngestService` reaches into 18+ private members of `ContextManager` including two `manager._orchestrator._xxx` chains. Acknowledged in plan; tighten in a follow-up. (maintainability, confidence 85) +- **M-03** [manual] `src/opencortex/context/manager.py:268-283` — Lazy import of `BenchmarkConversationIngestService` inside `__init__` is a structural signal the module boundary is in the wrong place; static type checkers cannot resolve types. (maintainability, confidence 75) +- **M-04** [manual] `RecompositionError` and `_BenchmarkRunCleanup` should move to a shared module (e.g., `context/benchmark_types.py` alongside the existing `recomposition_types.py`). Eliminates the lazy imports inside service methods. (maintainability, confidence 75) +- **M-05** [manual] `src/opencortex/context/benchmark_ingest_service.py:102-104` — `ingest_shape` validation duplicates what `Literal['merged_recompose', 'direct_evidence']` on the Pydantic request model could enforce at the boundary. (maintainability, confidence 70) +- **PERF-01** [manual] `src/opencortex/context/manager.py:2699-2728` — `_generate_session_summary` issues two independent full-scroll passes (`load_directories` then `load_merged`) over identical session data. Add `load_all` repository method that returns the full record set and let the caller partition by layer. Hits both benchmark and production session_end. (performance, confidence 90) +- **PERF-02** [manual] `src/opencortex/context/session_records.py:254-271,295-301` — `source_uri` filtered in-memory after full scroll; should be pushed into Qdrant filter via indexed `meta.source_uri` payload field. (performance, confidence 85) +- **PERF-03** [manual] `src/opencortex/context/benchmark_ingest_service.py:640-655` — `_ingest_direct_evidence` N+1: one `_get_record_by_uri` per segment after `add()` to re-fetch as dict. Use the returned `Context` directly. 10-100ms per ingest call on critical path. (performance, confidence 88) +- **REL-02** [manual] `src/opencortex/context/benchmark_ingest_service.py:522-528` — `cleanup.summary_uri = manager._generate_session_summary(...)` has no try/except. Mid-call failure after primary `add()` succeeds creates an orphaned summary record. (reliability, confidence 75) +- **REL-03** [gated_auto] `src/opencortex/context/benchmark_ingest_service.py:678-688` — `_ingest_direct_evidence` cleanup uses `contextlib.suppress(Exception)`; second cancellation arriving during `_delete_immediate_families` escapes (CancelledError is BaseException since 3.8). The KP-09 fix consolidated the outer except to BaseException; consider doing the same for the inner suppress. (reliability, confidence 75) +- **REL-04** [gated_auto] `src/opencortex/http/admin_routes.py:281` — `BenchmarkConversationIngestResponse.model_validate(result)` is not in the surrounding try/except. Schema drift surfaces as unstructured 500 with no session_id context logged. (reliability, confidence 75) +- **AC-03** [gated_auto] `src/opencortex/http/models.py:407` + `src/opencortex/context/manager.py:1492` — `event_date` default empty-string vs null inconsistency. Tied to KP-01 (already applied). Decide whether `_export_memory_record` should emit `None` instead of `''` for absent dates. (api-contract, confidence 75) +- **AC-04** [gated_auto] `src/opencortex/http/models.py:408` — `msg_range: Optional[List[int]]` accepts arbitrary-length lists; contract is always length-2. Add `Field(min_length=2, max_length=2)` to surface storage corruption at the HTTP layer. (api-contract, confidence 72) +- **KP-05** [manual] Shape strings `"merged_recompose"` / `"direct_evidence"` are bare literals at 6+ sites across two modules. Module-level constants. (kieran-python, confidence 85) +- **KP-07** [manual] `src/opencortex/context/session_records.py:242,279` — `load_merged` and `load_directories` share identical 15-line filter+sort bodies. Extract `_load_layer_records(session_id, layer, ...)` helper. (kieran-python, confidence 75) +- **ADV-U-003** [manual] Cancellation timing — if `CancelledError` fires during `_persist_rendered_conversation_source` BEFORE entering the cleanup-tracker scope, the source URI is created but untracked. (adversarial, confidence 50) +- **ADV-U-005** [manual] Cascade with ADV-U-001 + `contextlib.suppress(Exception)` wrapping `_purge_torn_benchmark_run` — partial purge failure becomes invisible; next ingest writes new leaves on top, marks run_complete=True, every retry returns the polluted set as authoritative. (adversarial, confidence 60) +- **ADV-U-006** [manual] Second cancellation mid-`compensate` orphans the tail of `merged_uris`/`directory_uris`. (adversarial, confidence 55) + +### P3 / Advisory (8 findings) + +- **correctness-003** [advisory] `src/opencortex/http/models.py:1184-1227` — `BenchmarkConversationIngestResponse` doesn't declare `extra='forbid'`; `model_validate` silently drops unknown keys. Either add ConfigDict or soften the U5 docstring claim about "drift detection". (correctness, confidence 50) +- **AC-05** [advisory] `src/opencortex/http/admin_routes.py:244,281` — Double Pydantic validation (explicit `model_validate` + FastAPI `response_model`). Negligible overhead on admin endpoint. (api-contract, confidence 90) +- **AC-06** [gated_auto] `src/opencortex/http/models.py:431-434` — `status: str` could be `Literal["ok"]` for machine-readable success-only invariant. (api-contract, confidence 70) +- **KP-08** [manual] `src/opencortex/context/benchmark_ingest_service.py:396` — `_bounded_complete` nested closure → static method for testability. (kieran-python, confidence 75) +- **PS-002** [manual] `src/opencortex/context/session_records.py` — `getattr + callable()` capability detection deviates from project's stated `hasattr` convention. (project-standards, confidence 75) +- **PERF-04** [advisory] Fallback scroll path issues 50K-row single query for adapters without scroll. No production impact (Qdrant has scroll). (performance, confidence 80) +- **PERF-05** [advisory] `model_validate` re-validates dict on response path. Sub-millisecond overhead. (performance, confidence 75) +- **REL-05** [manual] `src/opencortex/context/manager.py:278-283` — `BenchmarkConversationIngestService` import inside `__init__` runs eagerly at construction (not lazy). Import failure crashes every `ContextManager` instantiation. (reliability, confidence 50) + +## Notable Strengths (verified, not bugs) + +From adversarial review (Q1, Q2, Q7, Q8, Q10): +- Lock keying by `SessionKey = (collection, tenant, user, session_id)` correctly avoids false cross-tenant serialization. +- Sibling-cancel pattern in `_write_merged_leaves` is correct under Python 3.10+ cooperative scheduling. +- `_delete_immediate_families` works on benchmark_evidence URIs because `remove_by_uri` deletes by URI prefix (no layer check). +- Lazy import of `BenchmarkConversationIngestService` is safe — module body fully loads before instance construction (no deadlock). +- `raise exc.original from exc` preserves original traceback correctly. + +From agent-native review: +- `BenchmarkConversationIngestResponse` improves OpenAPI discoverability. +- 507 error body is structured (reason, session_id, method, count_at_stop, next_cursor, hint) — agent-parseable. +- Admin gate on benchmark endpoint is intentional and correct. From 29c3f5a150224fda35b8a00c266b428e0dc9c65e Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 14:40:17 +0800 Subject: [PATCH 9/9] fix(benchmark): close P1 review residuals (plan 005, U7-U10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the four highest-ROI P1 findings from the CE autofix run on plan 005 (commit 3e24971 covered the safe_auto pass; this is the gated_auto + manual layer that needs human judgment). - AC-02 (api-contract): benchmarks/oc_client.py now excludes 507 from the retryable status set. SessionRecordOverflowError is deterministic — the session record count cannot decrease on retry — so the prior status>=500 rule wasted ~4 minutes of backoff per affected session. One-line fix in _is_retryable_http_error. - REL-01 (reliability): _purge_torn_benchmark_run no longer swallows SessionRecordOverflowError via its generic `except Exception`. An overflow now raises out of the purge with a structured error log pointing operators at session_id rotation / manual paging, instead of silently treating the directory list as empty and leaving the failed prior run's directory records as permanent storage orphans. - ADV-U-001 (adversarial): _purge_torn_benchmark_run now passes tenant_id / user_id to load_directories. Every other repo call site threads scope; the missing kwargs here meant a torn-replay could, in principle, hand cross-tenant directory URIs to _delete_immediate_families (a hard-delete path with no layer check). Pin scope so the purge can never touch another tenant. - correctness-001 (correctness): _idempotent_hit_response wraps load_summary() in try/except. Restores the legacy degradation behavior — a transient storage error on the summary point lookup now logs and falls back to summary_uri=None instead of failing the entire idempotent replay. Parity with the pre-refactor _get_record_by_uri call path. - T-01 / T-02 / T-03 (testing): new tests/test_benchmark_ingest_service.py exercises the service in isolation. Covers eight cases the lifecycle / HTTP tests cannot reach: * _normalize_segments empty / whitespace / meta copy semantics * unsupported ingest_shape -> ValueError * empty-segments early-exit response shape * idempotent-hit summary lookup transient failure -> degrade * RecompositionError drain into cleanup tracker (existing test raises RuntimeError directly, bypassing the wrapper) * sibling-cancel-on-first-derive-failure (existing test only injects failure after all derives complete) Verification: - tests.test_benchmark_ingest_service (8) ✅ — new file - tests.test_session_records_repository (11) ✅ - tests.test_benchmark_ingest_response_model (5) ✅ - tests.test_http_server / test_locomo_bench / test_benchmark_ingest_lifecycle ✅ - tests.test_e2e_phase1 + tests.test_context_manager — only the pre-existing test_update_regenerates_fact_points_after_content_change failure remains (unrelated). Total: 193 tests, 1 pre-existing failure. Co-Authored-By: Claude Opus 4.7 (1M context) --- benchmarks/oc_client.py | 7 + .../context/benchmark_ingest_service.py | 24 +- src/opencortex/context/manager.py | 31 ++ tests/test_benchmark_ingest_service.py | 496 ++++++++++++++++++ 4 files changed, 557 insertions(+), 1 deletion(-) create mode 100644 tests/test_benchmark_ingest_service.py diff --git a/benchmarks/oc_client.py b/benchmarks/oc_client.py index 9ca9e1a..0cd8393 100644 --- a/benchmarks/oc_client.py +++ b/benchmarks/oc_client.py @@ -18,6 +18,13 @@ def _is_retryable_http_error(exc: Exception) -> bool: return True if isinstance(exc, httpx.HTTPStatusError): status = exc.response.status_code + # 507 Insufficient Storage is raised by SessionRecordOverflowError + # (REVIEW closure tracker AC-02). The condition is deterministic — + # the session record count cannot decrease on retry — so retrying + # 8 times with backoff just wastes ~4 minutes per affected + # session. Surface the error immediately instead. + if status == 507: + return False return status == 429 or status >= 500 return False diff --git a/src/opencortex/context/benchmark_ingest_service.py b/src/opencortex/context/benchmark_ingest_service.py index 12eea51..5dce828 100644 --- a/src/opencortex/context/benchmark_ingest_service.py +++ b/src/opencortex/context/benchmark_ingest_service.py @@ -259,10 +259,32 @@ async def _idempotent_hit_response( # Genuine idempotent hit: surface the prior run's summary URI # if one was persisted (REVIEW F4 / api-contract-005). + # + # REVIEW closure tracker correctness-001: the legacy code path + # used ``_get_record_by_uri`` which catches ``Exception`` and + # returns ``None`` on transient storage failure, so summary + # lookup failure simply degraded to ``summary_uri=None`` instead + # of failing the whole idempotent replay. ``_repo.load_summary`` + # now propagates errors directly. Wrap so summary lookup retains + # its tolerant degradation semantics — the idempotent response + # is still useful when storage hiccups on a single point lookup, + # and the next ingest will surface the real issue if it + # persists. existing_summary_uri = manager._session_summary_uri( tenant_id, user_id, session_id ) - existing_summary = await self._repo.load_summary(existing_summary_uri) + try: + existing_summary = await self._repo.load_summary(existing_summary_uri) + except Exception: + logger.warning( + "benchmark_ingest_conversation: idempotent hit summary " + "lookup failed sid=%s summary_uri=%s — degrading to " + "summary_uri=None", + session_id, + existing_summary_uri, + exc_info=True, + ) + existing_summary = None summary_uri_for_response = ( existing_summary_uri if existing_summary else None ) diff --git a/src/opencortex/context/manager.py b/src/opencortex/context/manager.py index a3ec949..f66574a 100644 --- a/src/opencortex/context/manager.py +++ b/src/opencortex/context/manager.py @@ -33,6 +33,7 @@ ) from opencortex.context.recomposition_types import RecompositionEntry from opencortex.context.session_records import ( + SessionRecordOverflowError, SessionRecordsRepository, record_msg_range, record_text, @@ -1377,11 +1378,41 @@ async def _purge_torn_benchmark_run( for rec in merged_records if rec.get("uri") ] + # REVIEW closure tracker ADV-U-001: every other repo call site + # threads (tenant_id, user_id) through so cross-tenant + # ``session_id`` collisions cannot bleed across scope. Without + # the kwargs the directory query loaded *every* directory record + # carrying this ``session_id`` regardless of who owned it, and + # the URI list flowed straight into ``_delete_immediate_families`` + # — a hard-delete path that walks URI prefixes without a layer + # check. Pin scope here so torn-replay purge can never touch + # another tenant's records. + # + # REVIEW closure tracker REL-01: the outer ``except Exception`` + # used to also swallow ``SessionRecordOverflowError`` (subclass + # of Exception). That meant a directory query exceeding the + # repo's safety cap would silently treat the directory list as + # empty, leaving every directory-layer record from the failed + # prior run as a permanent storage orphan with no operator + # signal. Surface overflow explicitly so the admin route maps it + # to 507 instead. try: directory_records = await self._session_records.load_directories( session_id=session_id, source_uri=source_uri, + tenant_id=tenant_id, + user_id=user_id, + ) + except SessionRecordOverflowError: + logger.error( + "benchmark_ingest: torn-run purge aborted — directory " + "query exceeded safety cap sid=%s source=%s. Directory " + "records from the failed prior run remain in storage; " + "rotate session_id or page manually before re-ingesting.", + session_id, + source_uri, ) + raise except Exception: # pragma: no cover - defensive directory_records = [] directory_uris = [ diff --git a/tests/test_benchmark_ingest_service.py b/tests/test_benchmark_ingest_service.py new file mode 100644 index 0000000..d7c3809 --- /dev/null +++ b/tests/test_benchmark_ingest_service.py @@ -0,0 +1,496 @@ +"""Direct unit tests for BenchmarkConversationIngestService. + +REVIEW closure tracker T-01 / T-02 / T-03 — exercises the service in +isolation so the six responsibilities (normalize, idempotent_hit, +write_merged_leaves with sibling-cancel, recompose+summarize with +RecompositionError drain, build_response, ingest_direct_evidence) have +direct coverage that does not require the full ASGI stack. + +The existing benchmark lifecycle / HTTP tests run the same code through +the public route, but they cannot reach a few branches: + +- ``ValueError`` on unsupported ``ingest_shape`` (line 110 of the + service) never fires from the route because the request DTO accepts + any string and the service is the validator. +- Empty-segments early-exit returns before touching storage; HTTP + tests with empty segments hit the legacy 410 shim instead. +- ``except RecompositionError`` drain branch needs the wrapper + exception specifically — existing lifecycle tests inject + ``RuntimeError`` and bypass the wrapper. +- Sibling-cancel-on-first-derive-failure runs after every leaf write + has already returned in the lifecycle tests. + +These tests intentionally use minimal in-memory fakes for the manager +dependencies the service touches. The service borrows ~15 manager +helpers via ``self._manager.X``; the fakes provide just the surface +each scenario exercises so the test failure mode is "service contract +broke" rather than "manager mock is wrong." +""" + +from __future__ import annotations + +import asyncio +import unittest +from dataclasses import dataclass, field +from typing import Any, Dict, List, Optional, Tuple + +from opencortex.context.benchmark_ingest_service import ( + BenchmarkConversationIngestService, +) +from opencortex.context.manager import RecompositionError, _BenchmarkRunCleanup + + +@dataclass +class _FakeContext: + """Minimal stand-in for the Context object orchestrator.add() returns.""" + + uri: str + + +class _FakeOrchestrator: + """Records add() / _complete_deferred_derive() calls for assertions.""" + + def __init__(self) -> None: + self.add_calls: List[Dict[str, Any]] = [] + self.derive_calls: List[Dict[str, Any]] = [] + self._records: Dict[str, Dict[str, Any]] = {} + + async def add(self, **kwargs: Any) -> _FakeContext: + self.add_calls.append(kwargs) + uri = kwargs["uri"] + self._records[uri] = { + "uri": uri, + "content": kwargs.get("content", ""), + "meta": dict(kwargs.get("meta") or {}), + } + return _FakeContext(uri=uri) + + async def _complete_deferred_derive(self, **kwargs: Any) -> None: + self.derive_calls.append(kwargs) + + async def _get_record_by_uri(self, uri: str) -> Optional[Dict[str, Any]]: + return self._records.get(uri) + + async def remove(self, uri: str) -> None: + self._records.pop(uri, None) + + +class _FakeManager: + """Manager surface the service borrows from. Just enough for the test.""" + + def __init__(self) -> None: + self._orchestrator = _FakeOrchestrator() + self._session_locks: Dict[Tuple[str, ...], asyncio.Lock] = {} + self._derive_semaphore = asyncio.Semaphore(4) + + # --- session-key plumbing --- + def _make_session_key( + self, tenant_id: str, user_id: str, session_id: str + ) -> Tuple[str, str, str, str]: + return ("collection", tenant_id, user_id, session_id) + + def _touch_session(self, _sk: Tuple[str, ...]) -> None: + pass + + def _remember_session_project(self, _sk: Tuple[str, ...]) -> None: + pass + + # --- source persist (fake — returns deterministic URI) --- + async def _persist_rendered_conversation_source( + self, + *, + session_id: str, + tenant_id: str, + user_id: str, + transcript: List[Dict[str, Any]], + enforce_transcript_hash: bool, + ) -> str: + return f"opencortex://{tenant_id}/{user_id}/session/conversations/{session_id}/source" + + # --- merged-leaf URI builders --- + def _merged_leaf_uri( + self, + tenant_id: str, + user_id: str, + session_id: str, + msg_range: List[int], + ) -> str: + return ( + f"opencortex://{tenant_id}/{user_id}/memories/events/" + f"{session_id}-{msg_range[0]:06d}-{msg_range[1]:06d}" + ) + + @staticmethod + def _benchmark_evidence_uri( + tenant_id: str, + user_id: str, + session_id: str, + segment_index: int, + msg_range: List[int], + ) -> str: + return ( + f"opencortex://{tenant_id}/{user_id}/memory/events/{session_id}/" + f"benchmark_evidence_{segment_index}_{msg_range[0]}_{msg_range[1]}" + ) + + # --- recomposition entry builders --- + def _benchmark_recomposition_entries( + self, + normalized_segments: List[List[Dict[str, Any]]], + ) -> List[Dict[str, Any]]: + # One entry per segment with deterministic msg_range. + next_index = 0 + entries: List[Dict[str, Any]] = [] + for segment in normalized_segments: + start = next_index + for _ in segment: + next_index += 1 + entries.append({ + "segment": segment, + "msg_range": [start, next_index - 1], + }) + return entries + + def _build_recomposition_segments( + self, + entries: List[Dict[str, Any]], + ) -> List[Dict[str, Any]]: + return [ + { + "messages": [str(m.get("content", "")) for m in entry["segment"]], + "msg_range": entry["msg_range"], + "source_records": [], + } + for entry in entries + ] + + async def _aggregate_records_metadata( + self, _records: List[Dict[str, Any]] + ) -> Dict[str, Any]: + return {} + + @staticmethod + def _decorate_message_text(text: str, _meta: Dict[str, Any]) -> str: + return text + + @staticmethod + def _benchmark_segment_meta(_segment: List[Dict[str, Any]]) -> Dict[str, Any]: + return {} + + # --- recompose + summary stubs (override per test) --- + async def _run_full_session_recomposition(self, **_kwargs: Any) -> List[str]: + return [] + + async def _generate_session_summary(self, **_kwargs: Any) -> Optional[str]: + return None + + async def _mark_source_run_complete(self, _source_uri: str) -> None: + pass + + # --- response building stubs --- + async def _hydrate_record_contents( + self, + records: List[Dict[str, Any]], + overrides: Optional[Dict[str, str]] = None, + ) -> Dict[str, str]: + merged = dict(overrides or {}) + for record in records: + uri = str(record.get("uri", "") or "") + if uri and uri not in merged: + merged[uri] = str(record.get("content", "") or "") + return merged + + @staticmethod + def _export_memory_record( + record: Dict[str, Any], + *, + hydrated_content: str = "", + ) -> Dict[str, Any]: + return { + "uri": record.get("uri", ""), + "content": hydrated_content, + "meta": dict(record.get("meta") or {}), + } + + def _session_summary_uri( + self, tenant_id: str, user_id: str, session_id: str + ) -> str: + return f"opencortex://{tenant_id}/{user_id}/session/{session_id}/summary" + + async def _delete_immediate_families(self, _uris: List[str]) -> None: + pass + + +class _FakeRepo: + """Repository stand-in that lets each test seed return values.""" + + def __init__(self) -> None: + self.merged_records: List[Dict[str, Any]] = [] + self.summary_record: Optional[Dict[str, Any]] = None + self.load_summary_should_raise: Optional[Exception] = None + + async def load_merged(self, **_kwargs: Any) -> List[Dict[str, Any]]: + return list(self.merged_records) + + async def load_summary(self, _uri: str) -> Optional[Dict[str, Any]]: + if self.load_summary_should_raise is not None: + raise self.load_summary_should_raise + return self.summary_record + + +class TestNormalizeSegments(unittest.TestCase): + """Pure static method — no service construction needed.""" + + def test_strips_empty_role_and_content(self): + segments = [ + [ + {"role": "user", "content": "hi"}, + {"role": "", "content": "no role"}, + {"role": "assistant", "content": ""}, + {"role": "user", "content": " "}, # whitespace-only + {"role": "assistant", "content": "ok"}, + ] + ] + normalized, transcript = ( + BenchmarkConversationIngestService._normalize_segments(segments) + ) + # Whitespace-only content is also stripped out. + self.assertEqual(len(normalized), 1) + self.assertEqual(len(normalized[0]), 2) + self.assertEqual(normalized[0][0]["role"], "user") + self.assertEqual(normalized[0][1]["role"], "assistant") + self.assertEqual(transcript, normalized[0]) + + def test_drops_segment_when_all_messages_empty(self): + segments = [ + [{"role": "", "content": ""}, {"role": "user", "content": ""}], + [{"role": "user", "content": "kept"}], + ] + normalized, transcript = ( + BenchmarkConversationIngestService._normalize_segments(segments) + ) + self.assertEqual(len(normalized), 1) + self.assertEqual(normalized[0][0]["content"], "kept") + self.assertEqual(len(transcript), 1) + + def test_returns_meta_dict_copy(self): + meta = {"event_date": "2026-04-25"} + segments = [[{"role": "user", "content": "hi", "meta": meta}]] + normalized, _ = ( + BenchmarkConversationIngestService._normalize_segments(segments) + ) + self.assertEqual(normalized[0][0]["meta"], meta) + # Mutating the input meta does not affect the normalized copy. + meta["event_date"] = "MUTATED" + self.assertEqual(normalized[0][0]["meta"]["event_date"], "2026-04-25") + + +class TestIngestDispatch(unittest.TestCase): + """Public ``ingest()`` entry point — shape validation + early exits.""" + + def _run(self, coro): + return asyncio.run(coro) + + def _service(self): + manager = _FakeManager() + repo = _FakeRepo() + return ( + BenchmarkConversationIngestService(manager=manager, repo=repo), + manager, + repo, + ) + + def test_unsupported_ingest_shape_raises_value_error(self): + async def check(): + service, _, _ = self._service() + with self.assertRaises(ValueError) as ctx: + await service.ingest( + session_id="s", + tenant_id="t", + user_id="u", + segments=[[{"role": "user", "content": "hi"}]], + ingest_shape="not_a_real_shape", + ) + self.assertIn("not_a_real_shape", str(ctx.exception)) + + self._run(check()) + + def test_empty_normalized_segments_returns_empty_response(self): + async def check(): + service, manager, _ = self._service() + response = await service.ingest( + session_id="empty", + tenant_id="t", + user_id="u", + segments=[ + [{"role": "", "content": ""}], + [{"role": "user", "content": " "}], + ], + ) + self.assertEqual( + response, + { + "status": "ok", + "session_id": "empty", + "source_uri": None, + "summary_uri": None, + "records": [], + }, + ) + # Nothing written to storage on the early-exit path. + self.assertEqual(manager._orchestrator.add_calls, []) + + self._run(check()) + + def test_idempotent_hit_summary_lookup_failure_degrades_to_none(self): + """REVIEW correctness-001 — transient storage error on summary + lookup must not fail the entire idempotent replay.""" + + async def check(): + service, manager, repo = self._service() + # Seed: prior records exist AND source is run_complete. + source_uri = ( + "opencortex://t/u/session/conversations/s/source" + ) + # Prime the orchestrator with the source record marked complete. + manager._orchestrator._records[source_uri] = { + "uri": source_uri, + "meta": {"run_complete": True}, + } + repo.merged_records = [ + { + "uri": "opencortex://t/u/memories/events/s-000000-000000", + "content": "prior", + "meta": {"layer": "merged", "msg_range": [0, 0]}, + } + ] + # Summary lookup raises a transient storage error. + repo.load_summary_should_raise = RuntimeError("transient blip") + + response = await service.ingest( + session_id="s", + tenant_id="t", + user_id="u", + segments=[[{"role": "user", "content": "hi"}]], + ) + # Must degrade gracefully rather than propagating. + self.assertEqual(response["status"], "ok") + self.assertIsNone(response["summary_uri"]) + self.assertEqual(len(response["records"]), 1) + + self._run(check()) + + +class TestRecompositionErrorDrain(unittest.TestCase): + """REVIEW T-03 — partial directory URIs must flow into the cleanup + tracker when ``_run_full_session_recomposition`` raises with the + proper wrapper. Existing lifecycle tests raise RuntimeError directly + and bypass this branch.""" + + def _run(self, coro): + return asyncio.run(coro) + + def test_recomposition_error_drains_partial_directory_uris(self): + async def check(): + manager = _FakeManager() + repo = _FakeRepo() + partial_dirs = [ + "opencortex://t/u/memories/events/s/dir-aa", + "opencortex://t/u/memories/events/s/dir-bb", + ] + + # Patch recompose to raise with partial URIs. + async def _failing_recompose(**_kwargs: Any) -> List[str]: + raise RecompositionError( + original=RuntimeError("disk full mid-recompose"), + created_uris=partial_dirs, + ) + + manager._run_full_session_recomposition = _failing_recompose + service = BenchmarkConversationIngestService( + manager=manager, repo=repo + ) + + cleanup = _BenchmarkRunCleanup(source_uri="opencortex://t/u/src") + with self.assertRaises(RuntimeError) as ctx: + await service._recompose_and_summarize( + session_id="s", + tenant_id="t", + user_id="u", + source_uri="opencortex://t/u/src", + include_session_summary=False, + cleanup=cleanup, + ) + self.assertIn("disk full", str(ctx.exception)) + # Drain happened: cleanup tracker now owns the partial dirs. + self.assertEqual(cleanup.directory_uris, partial_dirs) + + self._run(check()) + + +class TestSiblingCancelOnFirstDeriveFailure(unittest.TestCase): + """REVIEW T-02 — when one derive task raises mid-flight, every other + in-flight derive task must be cancelled before the exception + propagates to the cleanup tracker. The lifecycle test injects + failure AFTER all derives complete and never reaches this branch.""" + + def _run(self, coro): + return asyncio.run(coro) + + def test_sibling_derive_tasks_cancelled_when_first_fails(self): + async def check(): + manager = _FakeManager() + repo = _FakeRepo() + + # Track which sibling derives observe cancellation. + cancelled_uris: List[str] = [] + failed_uri = "opencortex://t/u/memories/events/s-000001-000001" + + async def _selective_derive(**kwargs: Any) -> None: + uri = kwargs["uri"] + if uri == failed_uri: + # Brief await so siblings actually start before raising. + await asyncio.sleep(0) + raise RuntimeError(f"derive blew up on {uri}") + # Sibling — block long enough that the failed task has + # time to raise and trigger cancellation. + try: + await asyncio.sleep(5) + except asyncio.CancelledError: + cancelled_uris.append(uri) + raise + + manager._orchestrator._complete_deferred_derive = _selective_derive + + service = BenchmarkConversationIngestService( + manager=manager, repo=repo + ) + cleanup = _BenchmarkRunCleanup(source_uri="opencortex://t/u/src") + + # Two-segment payload so we get one failing leaf and one sibling. + normalized = [ + [{"role": "user", "content": "first"}], + [{"role": "user", "content": "second"}], + ] + + with self.assertRaises(RuntimeError) as ctx: + await service._write_merged_leaves( + session_id="s", + tenant_id="t", + user_id="u", + source_uri="opencortex://t/u/src", + normalized_segments=normalized, + cleanup=cleanup, + ) + self.assertIn("derive blew up", str(ctx.exception)) + # Sibling observed cancellation — the gather() loop did its + # job (REVIEW F1 / REL-01 / ADV-001 guarantee). + self.assertGreaterEqual(len(cancelled_uris), 1) + # Both leaves were written (cleanup tracker holds them). + self.assertEqual(len(cleanup.merged_uris), 2) + + self._run(check()) + + +if __name__ == "__main__": + unittest.main()