Skip to content

refactor(context): server-side design patterns — Service + Repository + DTO (plan 005) - #7

Merged
HugoSkylight merged 9 commits into
masterfrom
refactor/server-side-design-patterns
Apr 25, 2026
Merged

refactor(context): server-side design patterns — Service + Repository + DTO (plan 005)#7
HugoSkylight merged 9 commits into
masterfrom
refactor/server-side-design-patterns

Conversation

@HugoSkylight

@HugoSkylight HugoSkylight commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Server-side refactor of the benchmark conversation ingest pipeline per plan 005 (REVIEW §25 Phases 3 + 5 + 6). Extracts the ~390-line ContextManager.benchmark_ingest_conversation body into three patterns: Service Layer, Repository / Gateway, DTO. Per §25.2 abstraction guardrails, only these three patterns were warranted; no Strategy / Abstract Factory.

Branched off master @ 155b218 (post-PR #6); rebased onto dfe0457 (post-PR #5).

What changed

Layer Module Role
Repository src/opencortex/context/session_records.py (new) SessionRecordsRepository wraps _load_session_merged_records, _load_session_directory_records, _session_layer_counts, session_summary lookup. Adds (tenant_id, user_id) scope push-down, cursor pagination via storage.scroll, fallback to storage.filter, SessionRecordOverflowError (HTTP 507). Serves 7+3+2 call sites.
DTO src/opencortex/http/models.py BenchmarkConversationIngestResponse + BenchmarkConversationIngestRecord Pydantic models. Replaces the bare Dict[str, Any] return at the admin route.
Service src/opencortex/context/benchmark_ingest_service.py (new) BenchmarkConversationIngestService with six private methods: _normalize_segments, _idempotent_hit_response, _ingest_merged_recompose, _write_merged_leaves (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. _benchmark_ingest_direct_evidence deleted from manager (service owns it).
Wiring src/opencortex/http/admin_routes.py Declares response_model=BenchmarkConversationIngestResponse, validates the orchestrator's dict via model_validate(...).

manager.py shrinks ~480 lines net (376-line method body → 28-line shim, 115-line method deleted, ~30 lines of new wiring).

Closure tracker entries closed

  • §25 Phase 3 (Service Layer): R2-11 (SRP), R2-12 (cleanup ownership boundary), F14 (ContextManager bloat)
  • §25 Phase 5 (Repository): 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), R2-22 (observability hook surface)
  • §25 Phase 6 (DTO): R2-28 / R4-P2-10 (typed response model)
  • CE autofix run 20260425-130006: KP-01..09, correctness-002, PS-001 (8 safe_auto)
  • CE residual P1 closures (this PR's last commit): AC-02, REL-01, ADV-U-001, correctness-001, T-01/T-02/T-03

Commits

  1. dded8c9 U1 — Extract SessionRecordsRepository
  2. ab6711d U2 — Scope discipline + cursor pagination + 507 overflow guard
  3. 4f6f001 U3 — BenchmarkConversationIngestResponse Pydantic DTO
  4. 39ffd72 U4 — Extract BenchmarkConversationIngestService
  5. 85a0ce3 U5 — Wire DTO through admin route
  6. 3b0ca91 Mark plan checkboxes complete
  7. 3e24971 CE autofix — 8 safe_auto fixes (typing, future annotations, off-by-one, docs)
  8. d83cc53 Record residual review findings
  9. 29c3f5a Close 4 P1 review residuals + add direct service unit tests

Verification

Suite Tests Status
tests.test_session_records_repository 11 ✅
tests.test_benchmark_ingest_response_model 5 ✅
tests.test_benchmark_ingest_service (new) 8 ✅
tests.test_http_server full ✅
tests.test_locomo_bench 40 ✅
tests.test_benchmark_ingest_lifecycle full ✅
tests.test_e2e_phase1 + test_context_manager 118 ✅ except 1 pre-existing failure (test_update_regenerates_fact_points_after_content_change — unrelated to this PR)

Total: 193 tests run, 1 pre-existing failure.

Residual work (not blocking, see docs/residual-review-findings/refactor-server-side-design-patterns.md)

  • 8 P1 → closed in this PR (commit 29c3f5a)
  • 12 P2 manual / gated_auto remaining: PERF-01/02/03 (storage round-trip optimization), M-04 (move shared types to dedicated module), AC-04 (msg_range length constraint), KP-05/07 (constants + helper extraction), REL-02/03/04 (refinements to error handling depth), ADV-U-003/005/006 (cancellation timing windows). Best handled in follow-up PRs.
  • 8 P3 / advisory.

Test plan

  • Run uv run python3 -m unittest tests.test_session_records_repository tests.test_benchmark_ingest_response_model tests.test_benchmark_ingest_service tests.test_http_server tests.test_locomo_bench tests.test_benchmark_ingest_lifecycle — should be all green.
  • Run uv run python3 -m unittest tests.test_e2e_phase1 tests.test_context_manager — only the pre-existing test_update_regenerates_fact_points_after_content_change should fail.
  • Manually exercise the admin endpoint /api/v1/admin/benchmark/conversation_ingest with both merged_recompose and direct_evidence shapes; verify response shape matches BenchmarkConversationIngestResponse.
  • Trigger a 507 path by ingesting against a saturated session and confirm structured error body (reason / session_id / method / count_at_stop / next_cursor / hint) and that oc_client.py does NOT retry it.
  • Spot-check git log -- src/opencortex/context/manager.py to see the ~480-line net reduction.

🤖 Generated with Claude Code

HugoSkylight and others added 9 commits April 25, 2026 12:14
@HugoSkylight
refactor(context): extract SessionRecordsRepository (U1, plan 005)
dded8c9 
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.
@HugoSkylight
refactor(context): scope discipline + cursor pagination + overflow gu…
ab6711d 
…ard (U2, plan 005)

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``).
@HugoSkylight
feat(http): add BenchmarkConversationIngestResponse DTO (U3, plan 005)
4f6f001 
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.
@HugoSkylight @claude
refactor(context): extract BenchmarkConversationIngestService (U4, pl…
39ffd72 
…an 005)

§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) <noreply@anthropic.com>
@HugoSkylight @claude
feat(http): wire BenchmarkConversationIngestResponse through admin ro…
85a0ce3 
…ute (U5, plan 005)

§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) <noreply@anthropic.com>
@HugoSkylight @claude
docs(plan): mark units U1-U5 complete (plan 005)
3b0ca91 
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) <noreply@anthropic.com>
@HugoSkylight @claude
fix(review): apply autofix feedback (plan 005, run 20260425-130006)
3e24971 
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) <noreply@anthropic.com>
@HugoSkylight @claude
docs(review): record residual review findings
d83cc53 
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) <noreply@anthropic.com>
@HugoSkylight @claude
fix(benchmark): close P1 review residuals (plan 005, U7-U10)
29c3f5a 
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) <noreply@anthropic.com>
@HugoSkylight
HugoSkylight merged commit 1123030 into master Apr 25, 2026
1 check failed
This was referenced Apr 25, 2026
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@HugoSkylight