Skip to content

refactor(benchmarks): extract shared conversation_mapping helper (§25 Phase 7) - #8

Merged
HugoSkylight merged 7 commits into
masterfrom
refactor/shared-adapter-helper
Apr 25, 2026
Merged

refactor(benchmarks): extract shared conversation_mapping helper (§25 Phase 7)#8
HugoSkylight merged 7 commits into
masterfrom
refactor/shared-adapter-helper

Conversation

@HugoSkylight

@HugoSkylight HugoSkylight commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Server-side dedup refactor per plan 006 (REVIEW §25 Phase 7). Extracts benchmarks/adapters/conversation_mapping.py to consolidate ~169 lines (10.83% jscpd) shared between benchmarks/adapters/conversation.py (LongMemEvalBench) and benchmarks/adapters/locomo.py (LoCoMoBench). Closes REVIEW closure tracker entries R2-24, R4-P2-8, R4-P2-9, plus TG-3 (store/mcp URI mapping equivalence regression lock).

Branched off master @ 1123030 (post-PR #7 merge).

What changed

Layer Module Role
Helper module (new) benchmarks/adapters/conversation_mapping.py (227 lines) Public free functions consumed by ≥2 conversation-style adapters
Adapters wired benchmarks/adapters/conversation.py (-144 lines) + benchmarks/adapters/locomo.py (-138 lines) Import shared helpers via from benchmarks.adapters import conversation_mapping as cm

Eight helpers extracted:

  • 5 byte-identical free helpers: normalize_text_set, message_span, ranges_overlap, overlap_width, record_time_refs (leading underscore dropped per base.py convention)
  • memory_record_snapshot — was @staticmethod async, now free async function
  • map_session_uris with new return_all: bool = False kwarg — False matches conversation single-best, True matches locomo full-sorted
  • extract_records_by_uri — NEW abstraction over inline {str(r["uri"]): dict(r) for r in payload["records"] if r["uri"]} comprehension

Closure tracker entries closed

  • R2-24 — 169 lines duplicated, 10.83% jscpd ✅
  • R4-P2-8 — adapter store-branch duplication ✅
  • R4-P2-9 — adapter mapping helper duplication ✅
  • TG-3 — store/mcp URI mapping equivalence regression test ✅
  • CE autofix run 20260425-160458 — PS-001, KP-002, KP-003, KP-004, ADV-006-003, ADV-006-004 (6 safe_auto)

Commits

  1. 6afe023 U1 — Create conversation_mapping.py with 5 byte-identical free helpers
  2. 71adaa4 U2 — Add memory_record_snapshot + extract_records_by_uri
  3. 027d882 U3 — Add map_session_uris with return_all kwarg
  4. 5da680b U4 — Wire both adapters; delete local copies
  5. 45f295c U5 — Add TG-3 store/mcp equivalence regression test
  6. 6143e18 Mark plan 006 checkboxes complete
  7. fe52492 CE autofix — 6 safe_auto fixes (CLAUDE.md docs, walrus operator, lambda rename, page-size const, invariant comments)

Verification

Suite Tests Status
tests.test_conversation_mapping (new) 49 ✅
tests.test_locomo_bench 24 ✅
tests.test_beam_bench 5 ✅
tests.test_http_server 20 ✅
tests.test_benchmark_ingest_lifecycle full ✅
tests.test_benchmark_ingest_response_model 5 ✅
tests.test_session_records_repository 11 ✅
tests.test_benchmark_ingest_service 8 ✅

Total: 130+ tests, all green. No assertion changes required to existing benchmark tests — the refactor is byte-identical at the call-site boundary.

CE review verdict: Ready to merge

8 reviewers dispatched (correctness, testing, maintainability, project-standards, agent-native, learnings, adversarial, kieran-python). Zero P0/P1 findings. 6 safe_auto fixes applied in commit fe52492. Adversarial review walked all 10 attack scenarios and verified clean (sort tie-break determinism, no shadow imports, no test-stub regressions, no nested-dict mutation aliasing).

Residual findings (not blocking — track for follow-ups)

P2 (1)

  • M-01 [maintainability, conf 50] — map_session_uris(return_all=...) boolean flag: consider renaming to return_candidates or splitting into map_session_uris (single-best) + map_session_uri_candidates (full list) for clearer dual-contract signaling. Documentation is adequate today; not a structural defect.

P3 / Advisory (10)

  • TF-1 [testing] — not time_refs guard inside map_session_uris time_refs fallback is not exercised by any test (requires record with no msg_range AND session with empty time_refs).
  • TF-3 [testing] — No test asserts the deleted private helpers (_normalize_text_set etc.) are absent from conversation.py / locomo.py. Consider a grep-style assertion.
  • TG-KP-001 — No test for record spanning ≥3 sessions simultaneously.
  • TG-KP-002 — Time_refs fallback test uses records with no msg_range; no test for record with non-overlapping msg_range routed through unmatched_records.
  • KP-001 [kieran-python] — Add ruff --select F401 to CI to catch dead imports automatically.
  • KP-005 — Mixed style: from __future__ import annotations is set but annotations still use typing.Dict/List/... instead of built-in dict[]/list[]. Project-wide convention shift recommended.
  • KP-006 — oc: Any could be a Protocol to harden the memory_list contract. Adapter-layer-wide pattern; this is the most impactful place to draw the line.
  • KP-007 — SPDX header inconsistency: conversation_mapping.py is one of few files in benchmarks/ with # SPDX-License-Identifier: Apache-2.0. Standardize either way.
  • ADV-006-001 [adversarial, conf 50] — extract_records_by_uri silently absorbs payload["records"]=None where the legacy comprehension would raise TypeError. No live trigger today (Pydantic DTO at src/opencortex/http/models.py:451 guarantees records: List[...] = Field(default_factory=list)).
  • ADV-006-002 [adversarial, conf 75] — memory_record_snapshot inherits a sticky-page-size infinite loop from both legacy implementations (terminates only on len(results) < limit, no max-iteration guard). Pre-existing flaw; extraction concentrates blast radius. Add max_pages budget.

Out of scope (deferred to separate PRs)

  • benchmarks/adapters/beam.py — has the same inline {uri: dict(record)} comprehension at lines 179-183 but doesn't share the other 7 helpers. Adopt extract_records_by_uri once stable.
  • R2-33 — turn_id naming + 0/1-based offset divergence between adapters. Convention-pinning, orthogonal to dedup.
  • F3 / ADV-003 — _lme_session_to_uri non-determinism under U13 concurrency. LongMemEval-specific; depends on a deterministic ordering decision still open.

Test plan

  • uv run python3 -m unittest tests.test_conversation_mapping tests.test_locomo_bench tests.test_beam_bench — should be all green
  • uv run python3 -m unittest tests.test_http_server tests.test_benchmark_ingest_lifecycle — verify no regressions in HTTP / lifecycle paths
  • Manual spot-check: wc -l benchmarks/adapters/conversation.py benchmarks/adapters/locomo.py shows -144 / -138 line deltas
  • Manual: grep -E "self\._(normalize_text_set|message_span|ranges_overlap|overlap_width|record_time_refs|memory_record_snapshot|map_session_uris)" benchmarks/adapters/{conversation,locomo}.py returns nothing

🤖 Generated with Claude Code

HugoSkylight and others added 7 commits April 25, 2026 15:35
@HugoSkylight
refactor(benchmarks): create conversation_mapping module with 5 free …
6afe023 
…helpers (U1, plan 006)

§25 Phase 7 — establish benchmarks/adapters/conversation_mapping.py
as the shared home for mapping helpers used by ≥2 conversation-style
adapters. This unit lifts the 5 byte-identical free helpers from
conversation.py:29-81 / locomo.py:170-284:

- normalize_text_set (was _normalize_text_set)
- message_span (was _message_span)
- ranges_overlap (was _ranges_overlap)
- overlap_width (was _overlap_width)
- record_time_refs (was _record_time_refs)

Drop leading underscore — module is public API for sibling modules,
matches the benchmarks/adapters/base.py convention.

The two adapter copies still exist; U4 will replace the call sites
and delete the local copies. The integration tests cannot regress
yet because nothing imports the new module.

New tests/test_conversation_mapping.py — 28 tests covering happy
paths, edge cases (boundary inclusivity, empty inputs, type guards),
and meta/abstract_json/event_date drain ordering for record_time_refs.
@HugoSkylight
refactor(benchmarks): add memory_record_snapshot + extract_records_by…
71adaa4 
…_uri (U2, plan 006)

Lifts the byte-identical _memory_record_snapshot staticmethod from
both adapters into a free async function taking oc directly. Adds
extract_records_by_uri as a pure abstraction over the inline
{str(r['uri']): dict(r) for r in payload['records'] if r['uri']}
comprehension that appears in every store-path and mainstream-path
response handler.

extract_records_by_uri is robust against:
- Missing 'records' key (returns {})
- Empty / None records list (returns {})
- Records with empty/missing/non-string uri (filtered out)
- Returns shallow copies so callers can mutate without aliasing

Tests cover happy paths, edge cases, and the snapshot pagination
loop (verifies it pages until results.length < limit). 11 new tests;
39 total in test_conversation_mapping.
@HugoSkylight
refactor(benchmarks): add map_session_uris with return_all kwarg (U3,…
027d882 
… plan 006)

Lifts the structurally-identical _map_session_uris from both adapters
into a single function that exposes the divergent return shape via a
keyword-only kwarg:

- return_all=False (default — matches LongMemEvalBench): returns
  [best_uri] per session, the tightest candidate after sort
- return_all=True (matches LoCoMoBench): returns the full sorted
  candidate list per session, defers tie-break to caller
  (LoCoMoBench._select_best_session_uri does question-aware refinement)

The two-pass mapping (span-based primary, time_refs fallback for
unmatched sessions) is unchanged from both legacy implementations.
Sort key (-overlap_width, width, span_start, uri_lex) is preserved.

Critical test reproduces test_ingest_prefers_tightest_overlapping_merged_record
fixture from tests/test_locomo_bench.py:197-235 at the helper level
so the locomo-side semantic is locked in this module's own tests
(distinct from the U5 integration test which exercises store-vs-mcp
path equivalence).

8 new tests; 47 total in test_conversation_mapping.
@HugoSkylight
refactor(benchmarks): wire both adapters to conversation_mapping; del…
5da680b 
…ete local copies (U4, plan 006)

§25 Phase 7 closure. Both LongMemEvalBench (conversation.py) and
LoCoMoBench (locomo.py) now import the shared helpers from
benchmarks.adapters.conversation_mapping (aliased as 'cm').

Deleted from each adapter:
- 5 module-level free helpers (normalize_text_set, message_span,
  ranges_overlap, overlap_width, record_time_refs)
- _memory_record_snapshot @staticmethod
- _map_session_uris @classmethod

Replaced inline {str(r['uri']): dict(r) for r in payload['records']
if r['uri']} comprehensions with cm.extract_records_by_uri(payload)
at all five call sites (3 in conversation.py, 2 in locomo.py).

Call-site distinction preserved via the cm.map_session_uris
return_all kwarg:
- conversation.py: return_all=False (single-best per session)
- locomo.py: return_all=True (full sorted list, defers tie-break to
  _select_best_session_uri)

Adapter-specific comments preserved verbatim:
- include_session_summary=False perf comments in both store branches
- REL-04 asyncio.CancelledError per-item cancellation cite (the
  cascade itself stays in the adapters since it wraps adapter-specific
  orchestration, not helper calls)

Net line counts:
- conversation.py: 660 → 516 (-144)
- locomo.py: 763 → 625 (-138)
- conversation_mapping.py: 0 → 227 (+227 helpers + docstring)
- Adapter dedup: -282 lines from adapters, +227 in module = -55 net

Verification:
- tests.test_conversation_mapping (47) ✅
- tests.test_locomo_bench ✅ (no test changes — adapters expose same
  surface)
- tests.test_beam_bench ✅ (unaffected)
- tests.test_http_server / test_benchmark_ingest_lifecycle /
  test_benchmark_ingest_response_model / test_session_records_repository
  / test_benchmark_ingest_service ✅
- 128 tests total, all green

Closes REVIEW closure tracker R2-24, R4-P2-8, R4-P2-9.
@HugoSkylight
test(benchmarks): add TG-3 store/mcp URI mapping equivalence test (U5…
45f295c 
…, plan 006)

Closes TG-3 from the closure tracker. Locks the equivalence the
§25 Phase 7 refactor preserves: the store-path
(benchmark_conversation_ingest -> extract_records_by_uri ->
map_session_uris) and mcp-path (context_end -> memory_record_snapshot
-> set-diff -> map_session_uris) must produce identical session->URI
mappings for the same fixture.

Without this lock, a future change could silently mutate either path
and the divergence would only surface at benchmark scoring time
(slow signal).

Two new tests:
- test_store_and_mcp_paths_produce_equivalent_uri_mapping: 3-record
  fixture (sess1-tight, sess2-tight, cumulative) routed through both
  paths, asserts equality for both return_all=False and
  return_all=True shapes.
- test_empty_records_produces_empty_mapping_on_both_paths: edge case
  where neither path has records — the empty mapping must match
  too, since refactors often forget edge cases.

49 total tests in test_conversation_mapping (47 unit + 2 integration).

Verification:
- tests.test_conversation_mapping (49) ✅
- tests.test_locomo_bench + tests.test_beam_bench (78 total) ✅
- tests.test_http_server / test_benchmark_ingest_lifecycle /
  test_benchmark_ingest_response_model / test_session_records_repository
  / test_benchmark_ingest_service ✅
- 130 tests total
@HugoSkylight
docs(plan): mark units U1-U5 complete (plan 006)
6143e18 
All five implementation units shipped:
- U1 conversation_mapping module + 5 free helpers (6afe023)
- U2 memory_record_snapshot + extract_records_by_uri (71adaa4)
- U3 map_session_uris with return_all kwarg (027d882)
- U4 wire both adapters; delete local copies (5da680b)
- U5 TG-3 store/mcp equivalence regression test (45f295c)

Closes REVIEW closure tracker R2-24, R4-P2-8, R4-P2-9.
Net: -55 lines across adapters, 49 new helper unit tests, 130
total tests green.
@HugoSkylight @claude
fix(review): apply autofix feedback (plan 006, run 20260425-160458)
fe52492 
CE review autofix pass — 6 deterministic safe_auto findings applied.
Residual gated_auto / manual / advisory findings recorded in PR body.

Mechanical fixes:
- PS-001: CLAUDE.md gains a benchmarks/adapters/ block listing
  base.py, conversation_mapping.py, conversation.py, locomo.py,
  beam.py. Same pattern as plan 005's PS-001 fix that added the
  context/ modules.
- KP-002: extract_records_by_uri uses the walrus operator so the
  dict key and the truthiness guard share the same str() result —
  one evaluation per record instead of two.
- KP-003: map_session_uris sort-key lambda parameter renamed
  `item` → `candidate` so the type (uri, width, overlap,
  span_start) is signaled at the call site.
- KP-004: 500-record page-size in memory_record_snapshot extracted
  to module-level _MEMORY_LIST_PAGE_SIZE constant with rationale.
- ADV-006-003: tie-break invariant comment near the sort key
  documents that uri_lex makes ties deterministic regardless of
  dict iteration order.
- ADV-006-004: dead-branch invariant comment at the time_refs
  fallback documents that ``span is None`` for every record from
  ``unmatched_records`` so the ``span[0] if span ...`` ternary is
  structurally dead — kept verbatim for byte-equivalence with the
  legacy implementation.

Verification:
- tests.test_conversation_mapping (49) ✅
- tests.test_locomo_bench (24) ✅
- tests.test_beam_bench (5) ✅
- 78 tests total, all green

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@HugoSkylight
HugoSkylight merged commit f5359cb into master Apr 25, 2026
1 check failed
@HugoSkylight HugoSkylight mentioned this pull request Apr 25, 2026
Merged
3 tasks
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