From a39220f53e20d085c7cf98a26f47b20c365c3ae2 Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 10:52:34 +0800 Subject: [PATCH 1/3] fix(benchmark): close 6 P2 residuals from prior CE review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per docs/plans/2026-04-25-004-refactor-benchmark-ingest-p2-residuals-plan.md. Closes the remaining nice-to-have / contract-tightening items left after PRs #3 and #4 cleared the P0/P1 set on the benchmark offline ingest path. U1 — api-contract-001: legacy URL `/api/v1/benchmark/conversation_ingest` now returns HTTP 410 Gone with a `detail.new_url` pointing at the admin URL, instead of FastAPI's bare 404. The shim accepts every verb so any client probe surfaces the migration. New CHANGELOG.md captures the URL move plus the U6 hash-canonicalization side effect. U2 — api-contract-004: `bench-collections` admin routes raise HTTPException(detail=...) instead of returning JSONResponse({"error": ...}). All admin route error paths now use FastAPI's standard `{"detail": ...}` envelope. Drops the unused JSONResponse import. U3 — KP-01: `BenchmarkConversationMessage._meta_within_byte_budget` now uses orjson.dumps (project convention) instead of stdlib json.dumps. orjson always emits UTF-8 bytes so the byte-budget check is exact, no encode round-trip. New CJK regression in test_04g asserts the budget holds against UTF-8-heavy meta values. U4 — KP-06: `_hydrate_record_contents` accepts an `overrides` kwarg that short-circuits the FS read for URIs already in memory. Both the merged_recompose and direct_evidence response builders pass their in-memory write-time captures as overrides, replacing the prior 3-level fallback lookup chain at the comprehension site with a flat `hydrated.get(uri, "")`. U5 — KP-08 / R2-15: new RecompositionEntry TypedDict in src/opencortex/context/recomposition_types.py. All three construction sites (_benchmark_recomposition_entries, _build_recomposition_entries, the inline builder inside _run_full_session_recomposition) and key consumers (_build_anchor_clustered_segments, _build_recomposition_segments, _finalize_recomposition_segment, _seed_with) annotate against it. Pure type-shape change; no runtime behavior diff. U6 — ADV-006: `_hash_transcript` now canonicalizes lists of primitives inside meta values before hashing. Two transcripts that differ only in the ordering of `time_refs` produce the same SHA-256 digest, so benign benchmark replays no longer hit a false 409. Lists of dicts (e.g. `tool_calls`) keep their order — sequence is treated as semantic for those. New regression test pins the contract. Tests: 168/169 pass. Sole failure is the pre-existing flaky `tests.test_context_manager.test_update_regenerates_fact_points_after_content_change` documented in project memory and unrelated to this branch. Migration note (CHANGELOG): U6 changes hash semantics. Source records ingested before this release will see one-time 409 on first replay; delete the source URI or rotate session_id to recover. After that canonical-form replays remain idempotent. --- CHANGELOG.md | 46 +++ ...ctor-benchmark-ingest-p2-residuals-plan.md | 329 ++++++++++++++++++ src/opencortex/context/manager.py | 244 ++++++++----- src/opencortex/context/recomposition_types.py | 47 +++ src/opencortex/http/admin_routes.py | 15 +- src/opencortex/http/models.py | 12 +- src/opencortex/http/server.py | 33 ++ tests/test_benchmark_ingest_lifecycle.py | 75 ++++ tests/test_http_server.py | 150 ++++++++ 9 files changed, 848 insertions(+), 103 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 docs/plans/2026-04-25-004-refactor-benchmark-ingest-p2-residuals-plan.md create mode 100644 src/opencortex/context/recomposition_types.py diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..8baf7cc --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,46 @@ +# Changelog + +All notable changes to OpenCortex are documented here. The format follows +[Keep a Changelog](https://keepachangelog.com/en/1.1.0/) loosely, and dates +are ISO-8601 in UTC. + +Versions before this file existed are reconstructed from project memory and +git history; treat them as best-effort summaries rather than authoritative +records. + +## Unreleased + +### Changed + +- **Benchmark offline conversation ingest endpoint moved under the admin + namespace.** The endpoint `POST /api/v1/benchmark/conversation_ingest` + is now `POST /api/v1/admin/benchmark/conversation_ingest` and requires + an admin role JWT (`_require_admin()`). The legacy URL returns + **HTTP 410 Gone** with a JSON body pointing at the new path; the + shim is scheduled for removal in v0.8.0. Callers using the bundled + `benchmarks/oc_client.py` were updated automatically — only out-of-tree + consumers need to update their URL and ensure their token carries the + admin role. + +- **Admin route error envelopes standardized on `{"detail": ...}`.** The + pre-existing `bench-collections` admin endpoints previously returned + `{"error": ...}`; they now return `{"detail": ...}` to match every + other admin route (403 from `_require_admin`, 409 from transcript hash + conflict, 504 from server-side timeout). FastAPI's default exception + envelope is the canonical shape across the admin surface. + +- **`_hash_transcript` canonicalizes list ordering for benchmark + transcript hashing.** Previously, two transcripts that differed only + in the ordering of list values inside `meta` (e.g., reordered + `time_refs`) hashed to different SHA-256 digests, causing benign + benchmark replays to receive a false **HTTP 409 Conflict**. The hash + function now sorts lists of primitives recursively in the meta values + before serialization. Lists of dicts (e.g. `tool_calls`) keep their + original order — sequence is treated as semantic for those. + + **Migration note:** Source records ingested before this release used + the old hash algorithm. The first benchmark replay of any pre-existing + session will see a hash mismatch and receive 409. To resolve, either + delete the source record (`opencortex://{tenant}/{user}/session/conversations/{session_id}/source`) + or rotate the `session_id`. Subsequent replays follow the new + canonical-form hash and remain idempotent. diff --git a/docs/plans/2026-04-25-004-refactor-benchmark-ingest-p2-residuals-plan.md b/docs/plans/2026-04-25-004-refactor-benchmark-ingest-p2-residuals-plan.md new file mode 100644 index 0000000..df0bdd9 --- /dev/null +++ b/docs/plans/2026-04-25-004-refactor-benchmark-ingest-p2-residuals-plan.md @@ -0,0 +1,329 @@ +--- +title: "refactor: Close P2 residuals on benchmark offline ingest path" +type: refactor +status: active +date: 2026-04-25 +--- + +# refactor: Close P2 residuals on benchmark offline ingest path + +## Overview + +PRs #3 and #4 closed every P0 and P1 finding from the prior 4-round CE review of `feat/benchmark-offline-conv-ingest`. Five P2 items and one cheap P3 (`ADV-006`) remain on the residual sheet (`docs/residual-review-findings/feat-benchmark-offline-conv-ingest.md`). This plan closes them in a single PR — none requires architectural change, the touched surfaces are well-known from the prior work, and bundling avoids review thrash on six trivially-related fixes. + +--- + +## Problem Frame + +Six independent residuals on the merged benchmark ingest path. None blocks production use; together they tighten contracts, eliminate cross-codebase inconsistencies, and remove one real correctness footgun (`ADV-006` false 409s on benign list reordering). All cited code paths exist on master at HEAD `90eb069` after PRs #3/#4 merged. + +Source documents: +- Prior CE review: `.context/compound-engineering/ce-code-review/20260424-152926-6301c860/REVIEW.md` +- Follow-up review (after PR #3 work): `.context/compound-engineering/ce-code-review/20260425-093102-b5a311de/REVIEW.md` +- Residual handoff (canonical list): `docs/residual-review-findings/feat-benchmark-offline-conv-ingest.md` + +--- + +## Requirements Trace + +- **R1 (api-contract-001):** A request to the legacy URL `/api/v1/benchmark/conversation_ingest` returns a 410 Gone with a `Location`/`detail` pointing to the new admin URL, and the change is documented in a CHANGELOG entry. +- **R2 (api-contract-004):** All admin route error responses use a single envelope shape — either FastAPI's default `{"detail": ...}` or `{"error": ...}` — consistently across `_require_admin`, payload validation, timeout, conflict, and bench-collection paths. +- **R3 (KP-01):** The Pydantic `field_validator` on `BenchmarkConversationMessage.meta` uses `orjson` (project convention) instead of stdlib `json` for serialization. +- **R4 (KP-06):** The `benchmark_ingest_conversation` response builder no longer carries a 3-level lookup chain (in-memory map → fallback FS hydration → empty string). Hydration is unified in one helper that takes the merged record set and returns a complete URI→content map. +- **R5 (KP-08 / R2-15):** Recomposition entries are typed via a `RecompositionEntry` TypedDict. All three construction sites (`_build_recomposition_entries`, `_benchmark_recomposition_entries`, the inline builder inside `_run_full_session_recomposition`) produce that type, and consumers narrow against it. +- **R6 (ADV-006):** `_hash_transcript` produces the same digest for two transcripts that differ only in list-element ordering of meta values (e.g. `time_refs`), so benign replays do not return false 409s. + +--- + +## Scope Boundaries + +- Do not modify production conversation lifecycle (`context_commit`, `context_end`). +- Do not introduce a 12th MCP tool. Benchmark route stays admin-only HTTP. +- Do not change benchmark scoring methodology. +- Do not touch `docs/solutions/`, `docs/brainstorms/`, or other CE pipeline artifacts. +- Do not bundle the still-deferred U12 `add_batch` work — that needs its own plan. + +### Deferred to Follow-Up Work + +- Phase 2 behavior-preserving refactor (extract `BenchmarkConversationIngestService`, route relocation polish, observability via `StageTimingCollector`) — too big for this PR; warrants its own plan. +- The remaining nice-to-have residuals not in this plan (F3/ADV-003 cross-conv shared state non-determinism, KP-02/03/04/05/07 Pythonic style nits, M1-M4 maintainability, TEST-001/002/005/008 coverage gaps) — separate polish PR. + +--- + +## Context & Research + +### Relevant Code and Patterns + +- `src/opencortex/http/server.py` (post-PR-#3 state) — old benchmark route was *removed* in U1, so today a request to `/api/v1/benchmark/conversation_ingest` hits FastAPI's default 404 handler with no body context. The 410 shim has to be added back as an explicit route. +- `src/opencortex/http/admin_routes.py:240-289` — current admin benchmark handler (post-PR-#4); error envelopes today: 403 = string `detail`, 504 = string `detail`, 409 = dict `detail`. Pre-existing bench-collection routes at `:200, :213` use `JSONResponse({"error": ...}, status_code=400)` — a *third* style. +- `src/opencortex/http/models.py:316-331` — `BenchmarkConversationMessage._meta_within_byte_budget` uses stdlib `json.dumps(value, ensure_ascii=False, sort_keys=True)`. Project memory (`MEMORY.md`) records `orjson migration across codebase` completed 2026-03-22+. Other places in the same module already import orjson. +- `src/opencortex/context/manager.py:1150-1163` — `_hash_transcript` uses `orjson.dumps(normalized, option=orjson.OPT_SORT_KEYS)`. `OPT_SORT_KEYS` recurses into nested dicts but leaves list ordering unchanged — confirmed by the adversarial reviewer in the follow-up run. +- `src/opencortex/context/manager.py:1857-1893` — current 3-level hydration lookup (`merged_content_by_uri.get(uri, fallback_hydrated.get(uri, ""))`). +- `src/opencortex/context/manager.py:_benchmark_recomposition_entries`, `_build_recomposition_entries`, and the inline builder inside `_run_full_session_recomposition` — three call sites all returning `Dict[str, Any]` entries with the same shape (text, uri, msg_start, msg_end, token_count, anchor_terms, time_refs, source_record, immediate_uris, superseded_merged_uris). + +### Institutional Learnings + +- `docs/solutions/best-practices/single-bucket-scoped-probe-2026-04-16.md` — already cited in plan 003. Still informs U2 (error envelope consistency belongs to the same "narrow contract surface" discipline). +- `docs/solutions/best-practices/memory-intent-hot-path-refactor-2026-04-12.md` — same. The TypedDict change in U5 is a small instance of "make the contract typecheckable so drift hurts at compile time." +- No existing entry for **error envelope conventions**, **transcript hashing canonical-form rules**, or **TypedDict rollout patterns**. After merge, U2 and U6 are reasonable `/ce-compound` candidates if the team finds them useful. + +### External References + +External research skipped — every change has multiple direct local examples to mirror (orjson usage, Pydantic field_validator, `JSONResponse` patterns, TypedDict patterns elsewhere in the codebase). + +--- + +## Key Technical Decisions + +- **Error envelope choice (R2): standardize on FastAPI's `{"detail": ...}` for all admin routes.** The two existing string-detail responses (403/504) and the one dict-detail response (409) all flow through `HTTPException`, which already wraps the value as `{"detail": ...}`. The pre-existing bench-collection `JSONResponse({"error": ...})` calls become `raise HTTPException(status_code=400, detail=...)` for symmetry. Keeping the dict-detail shape on 409 (with `existing_hash`/`supplied_hash`) is fine — `detail` accepts strings or dicts. Choosing FastAPI's default avoids inventing a custom envelope class that would itself become the "fourth style". +- **410 Gone shim (R1): explicit route, not a middleware.** Adding a single route for `/api/v1/benchmark/conversation_ingest` (all methods) returning 410 with a `detail` and `Location` header is more discoverable than middleware-based path interception, and lets us drop the route after a release or two. +- **Hydration helper signature (R4):** unify on one async helper `_hydrate_record_contents_with_overrides(records, overrides_by_uri)` that returns a `dict[uri, content]`. Records whose URI is in `overrides` skip the FS read. The benchmark caller passes its captured in-memory map as `overrides`. Direct evidence path passes its own map. The 3-level lookup at the comprehension site collapses to a single `hydrated.get(uri, "")`. +- **TypedDict shape (R5):** `RecompositionEntry` carries the union of fields all three call sites currently produce — explicit `Required[...]` for fields every site sets, `NotRequired[...]` for the optional ones (`immediate_uris`, `superseded_merged_uris` are unconditional empty lists in two of the three sites; mark as `Required` so the compiler catches future drift). Define in `src/opencortex/context/recomposition_types.py` so it can be imported without circular deps. +- **Hash canonicalization (R6):** sort lists recursively during the normalization pass before serializing. Implemented as a small recursive helper `_canonicalize_for_hash(value)` that sorts lists of primitives in-place but leaves lists-of-dicts untouched (their order may be semantic — message ordering matters). Apply only to meta values, not to the transcript message list itself (message order IS semantic). + +--- + +## Open Questions + +### Resolved During Planning + +- **Should the 410 shim include a CORS preflight handler?** No. Admin endpoints don't need CORS; the legacy URL was admin-only by intent. +- **Should we also normalize the bench-collection error envelopes (pre-existing, not in this plan's residual set)?** Yes — they're in the same file and the inconsistency is what R2 explicitly calls out. The fix is a few lines. +- **Should `_canonicalize_for_hash` sort lists of dicts?** No. Lists of dicts in benchmark meta are uncommon, and order can be load-bearing (e.g., `tool_calls` sequence). Keeping the helper conservative avoids changing hash semantics for cases not flagged by ADV-006. + +### Deferred to Implementation + +- Exact `RecompositionEntry` field set — pinned by the 3 construction sites. May discover one site sets a field the others don't; surface as part of unit U5. +- Precise wording of the 410 detail string and CHANGELOG entry — short prose, not architectural. + +--- + +## Implementation Units + +- [ ] U1. **410 Gone shim + CHANGELOG for legacy benchmark URL** + +**Goal:** Requests to `/api/v1/benchmark/conversation_ingest` (all methods) return 410 with a structured detail pointing at the new admin URL. Document the URL change in `CHANGELOG.md` (or create one if absent). + +**Requirements:** R1. + +**Dependencies:** None. + +**Files:** +- Modify: `src/opencortex/http/server.py` (add a single route that raises `HTTPException(status_code=410, detail={...})` for the legacy path; mount before fall-through 404) +- Modify (or create): `CHANGELOG.md` — note the URL change with the deprecation date and target replacement +- Test: `tests/test_http_server.py` + +**Approach:** +- Register the legacy path as `@app.api_route(legacy_url, methods=["GET", "POST", "PUT", "PATCH", "DELETE"])` so any verb gets 410 (not just POST). Body: `{"detail": {"reason": "moved", "new_url": "/api/v1/admin/benchmark/conversation_ingest", "removed_in": "0.8.0"}}`. +- CHANGELOG entry under `## Unreleased` (or whatever convention exists if the file is already there). + +**Patterns to follow:** +- Existing FastAPI route registrations in `src/opencortex/http/server.py:_register_routes`. + +**Test scenarios:** +- Happy path: `POST /api/v1/benchmark/conversation_ingest` returns 410, response JSON has `detail.new_url == "/api/v1/admin/benchmark/conversation_ingest"`. +- Edge case: `GET` and `OPTIONS` to the same legacy path also return 410 (verbs don't matter for a removed endpoint). +- Edge case: the new admin URL is unaffected — `POST /api/v1/admin/benchmark/conversation_ingest` with admin role still returns 200 (regression lock). + +**Verification:** +- 410 surfaces consistently for the old URL; no other admin/business route is shadowed. + +--- + +- [ ] U2. **Standardize admin route error envelopes** + +**Goal:** Every admin route error path returns FastAPI's standard `{"detail": ...}` envelope. The pre-existing `JSONResponse({"error": ...})` returns in `bench-collections` are converted to `raise HTTPException(status_code=400, detail=...)`. + +**Requirements:** R2. + +**Dependencies:** None (independent of U1; can land in either order). + +**Files:** +- Modify: `src/opencortex/http/admin_routes.py` (change `JSONResponse({"error": ...})` to `HTTPException` raises in `create_bench_collection` and `delete_bench_collection`) +- Test: `tests/test_http_server.py` (add envelope-shape assertions for 400/403/409/504) + +**Approach:** +- Replace each `return JSONResponse({"error": "..."}, status_code=400)` with `raise HTTPException(status_code=400, detail="...")`. Verify response shape stays `{"detail": "..."}` (FastAPI's default exception handler). +- The 409 path's existing dict `detail` (`{"reason": ..., "session_id": ..., ...}`) is fine — `HTTPException.detail` is `Any`. No change there. +- Leave `_require_admin()` (403, string detail) and the 504 timeout (string detail) unchanged — they already use the standard envelope. + +**Patterns to follow:** +- Existing `HTTPException` raises in `admin_routes.py` (`_require_admin`, the 504 in `admin_benchmark_conversation_ingest`). + +**Test scenarios:** +- Happy path: `POST /api/v1/admin/collection` with non-admin → 403, body has `detail` key (string). +- Error path: `POST /api/v1/admin/collection` with bad name → 400, body has `detail` key (string), no `error` key. +- Error path: `DELETE /api/v1/admin/collection/foo` with non-`bench_` name → 400, same envelope. +- Regression: 409 hash-mismatch still surfaces `detail.existing_hash` and `detail.supplied_hash` (dict detail). + +**Verification:** +- `grep "JSONResponse" src/opencortex/http/admin_routes.py` shows zero hits inside route handlers. + +--- + +- [ ] U3. **Switch `models.py` field_validator to orjson** + +**Goal:** `BenchmarkConversationMessage._meta_within_byte_budget` uses `orjson.dumps` instead of stdlib `json.dumps`, matching the project-wide convention recorded in MEMORY.md. + +**Requirements:** R3. + +**Dependencies:** None. + +**Files:** +- Modify: `src/opencortex/http/models.py` +- Test: `tests/test_http_server.py` (existing payload-bounds test in `test_04g` already exercises the validator; add one assertion that a CJK-heavy meta value still passes the byte-budget check correctly under orjson, since `ensure_ascii=False` was the stdlib quirk being relied on) + +**Approach:** +- Drop `import json` at module top (or keep if used elsewhere in models.py — verify by grep). +- Replace `json.dumps(value, ensure_ascii=False, sort_keys=True)` with `orjson.dumps(value, option=orjson.OPT_SORT_KEYS)`. orjson always emits non-ASCII as UTF-8 bytes (no `ensure_ascii` flag needed) and supports `OPT_SORT_KEYS`. +- `len(serialized)` becomes `len(serialized)` directly — orjson returns bytes already, so the existing `.encode("utf-8")` cast is redundant; drop it. + +**Patterns to follow:** +- Other orjson usage in the codebase, e.g. `src/opencortex/context/manager.py` `import orjson as json`. + +**Test scenarios:** +- Edge case: meta dict with CJK characters near the 16 KB limit still validates correctly (the orjson-encoded bytes count, not the Python string length). +- Regression: existing `test_04g_benchmark_ingest_payload_bounds` passes (oversized meta still rejected with 422). + +**Verification:** +- No `json.dumps` call remains in `src/opencortex/http/models.py`. + +--- + +- [ ] U4. **Unify hydration lookup in benchmark response builder** + +**Goal:** Replace the 3-level `merged_content_by_uri.get(uri, fallback_hydrated.get(uri, ""))` lookup chain with a single helper call that returns a complete `dict[uri, content]` for all records being exported, consuming the in-memory write-time captures as overrides. + +**Requirements:** R4. + +**Dependencies:** None. + +**Files:** +- Modify: `src/opencortex/context/manager.py` (`_hydrate_record_contents` extension OR new `_hydrate_record_contents_with_overrides`; collapse the two response-build comprehensions in `benchmark_ingest_conversation` and `_benchmark_ingest_direct_evidence`) +- Test: `tests/test_benchmark_ingest_lifecycle.py` (existing tests cover content-non-empty assertion; no new test needed unless behavior changes — verify with regression run) + +**Approach:** +- Extend the existing helper signature: `_hydrate_record_contents(records, overrides=None)`. When `overrides` is provided, URIs found there short-circuit (no FS read), URIs not in overrides go through the existing FS read path, and the returned dict contains every URI from `records`. +- The benchmark merged_recompose path passes `overrides=merged_content_by_uri`. Direct evidence path passes `overrides=evidence_content_by_uri`. The comprehension collapses to `_export_memory_record(rec, hydrated_content=hydrated.get(uri, ""))`. +- Keep `_export_memory_record`'s `hydrated_content` kwarg unchanged — the simplification is at the call site, not the API. + +**Patterns to follow:** +- Existing `_hydrate_record_contents` shape (returns `dict[str, str]`, missing keys map to ""). + +**Test scenarios:** +- Happy path: merged_recompose response records all have non-empty `content` (existing test_04d assertion). +- Happy path: direct_evidence response records have non-empty `content` (existing test_04e assertion). +- Edge case: records whose URI is in `overrides` do not trigger a FS read (use a write-counter mock or assert via internal log). +- Regression: empty record set → empty dict, no FS calls. + +**Verification:** +- Both response builders use the same helper call shape; the 3-level fallback chain is gone. + +--- + +- [ ] U5. **Introduce `RecompositionEntry` TypedDict** + +**Goal:** All three recomposition-entry construction sites produce a `RecompositionEntry` TypedDict instead of bare `Dict[str, Any]`. Consumers (`_build_anchor_clustered_segments`, `_finalize_recomposition_segment`, the directory derive loop) annotate against it. + +**Requirements:** R5. + +**Dependencies:** None — this is a pure type-shape change with no behavioral implications. + +**Files:** +- Create: `src/opencortex/context/recomposition_types.py` (defines `RecompositionEntry` TypedDict) +- Modify: `src/opencortex/context/manager.py` (annotate `_benchmark_recomposition_entries`, `_build_recomposition_entries`, the inline builder inside `_run_full_session_recomposition`, and consumer signatures) +- Test: `tests/test_benchmark_ingest_lifecycle.py` AND `tests/test_context_manager.py` — no behavioral change, but a static type-check pass is the real verification + +**Approach:** +- New module `recomposition_types.py` to avoid pulling `manager.py` into a typing-only import cycle. +- Fields based on actual current shape: `text: Required[str]`, `uri: Required[str]`, `msg_start: Required[int]`, `msg_end: Required[int]`, `token_count: Required[int]`, `anchor_terms: Required[Set[str]]`, `time_refs: Required[Set[str]]`, `source_record: Required[Dict[str, Any]]`, `immediate_uris: Required[List[str]]`, `superseded_merged_uris: Required[List[str]]`. All currently set unconditionally at all three sites — if implementation reveals one site omits a field, mark `NotRequired` and document why. +- Add `from __future__ import annotations` at the top of `manager.py` if not already present (avoids runtime evaluation cost for the TypedDict reference). +- This is not a behavior change — pure type annotation. No new tests needed; existing tests stay green. + +**Execution note:** Run `mypy --strict src/opencortex/context/manager.py` (or whatever the project type-check incantation is) after the change to confirm the TypedDict catches drift. If the project doesn't run mypy, surface that in the verification. + +**Patterns to follow:** +- Other TypedDicts in the codebase if any exist; otherwise the standard PEP 589 form. + +**Test scenarios:** +- Test expectation: none -- pure type-shape change with no runtime behavior. Verification is via existing test suite remaining green plus, when available, a mypy / pyright pass. + +**Verification:** +- All three construction sites import and return `RecompositionEntry`. +- Consumers annotate parameter types accordingly. +- Existing test suite (95+ tests) stays green. + +--- + +- [ ] U6. **Sort lists in `_hash_transcript` to canonicalize benign reordering** + +**Goal:** `_hash_transcript` produces the same digest for two transcripts that differ only in the ordering of list values inside meta (e.g., `time_refs: ["2023-05-01", "9 am on 1 May"]` vs `["9 am on 1 May", "2023-05-01"]`). Eliminates false 409 conflicts on benign benchmark replays. + +**Requirements:** R6. + +**Dependencies:** None. + +**Files:** +- Modify: `src/opencortex/context/manager.py` (`_hash_transcript` + new `_canonicalize_for_hash` helper) +- Test: `tests/test_benchmark_ingest_lifecycle.py` (new test asserting hash equality for time_refs reordering; new test asserting hash inequality when message text differs) + +**Approach:** +- Add module-level `_canonicalize_for_hash(value)` that: + - For `dict`: recurse on values. + - For `list`: if every element is a primitive (str / int / float / bool / None), return `sorted(value)`. Otherwise (list of dicts) leave order intact — order may be load-bearing for `tool_calls`-style sequences. + - For everything else: return as-is. +- Apply to each message's `meta` field during normalization. Do NOT apply to the message list itself — message order is semantic. +- Keep `OPT_SORT_KEYS` for dict-key ordering (already there). + +**Patterns to follow:** +- Existing `_hash_transcript` structure. + +**Test scenarios:** +- Happy path: same transcript with `time_refs` list reordered → same hash → idempotent re-ingest (200, no 409). +- Edge case: meta with nested dict values reorders dict keys → same hash (already covered by OPT_SORT_KEYS, regression lock). +- Edge case: meta with `tool_calls` list (list of dicts) — order preserved, hash differs if order differs (correct: tool_call order is semantic). +- Error path: same `session_id` with genuinely different message content → still 409 (existing test_409_on_same_session_different_transcript stays green). + +**Verification:** +- New regression test demonstrates the false-409 case is fixed; the genuine-409 case still fires. + +--- + +## System-Wide Impact + +- **Interaction graph:** U1 adds a route to the business-routes app (parallel to admin routes — affects only the legacy URL). U2 changes response shape for a few error paths. U3 is internal validator only. U4-U5 are internal refactors. U6 changes a hash function used only by the benchmark idempotent-hit detector. +- **Error propagation:** U2 unifies envelopes — adapter / client code that reads `response.json()["detail"]` continues to work; any code reading `response.json()["error"]` for bench-collections breaks (none currently exists). +- **State lifecycle risks:** U6 changes hash semantics — a previously-stored source record's transcript_hash will not match a re-canonicalized fresh one. **Mitigation:** the F5 `run_complete` marker AND the hash check are both required for idempotent-hit. A pre-existing source record from before this change will not match the new canonical hash, so the next ingest will treat it as a hash mismatch (409). This is a single-time event per session_id and can be cleared by deleting the source record. Consider noting in CHANGELOG. +- **API surface parity:** U1 publicly documents the URL move. No other interface (MCP, CLI) needs an update — they all go through `OCClient` which already targets the new URL. +- **Integration coverage:** The lifecycle test suite (`tests/test_benchmark_ingest_lifecycle.py`) covers the AR3-AR7 contracts. New tests in U1, U2, U6 add coverage for the changed shapes. U4-U5 are pure refactors covered by existing tests. +- **Unchanged invariants:** Production conversation lifecycle (`context_commit`, `context_end`); MCP tool surface; benchmark scoring methodology; admin gate enforcement; cleanup tracker compensation order. + +--- + +## Risks & Dependencies + +| Risk | Mitigation | +|------|------------| +| **U6 hash change invalidates existing source records.** Sessions ingested before this PR will return 409 on first re-ingest after deploy. | Document in CHANGELOG. The 409 is recoverable: delete the source record OR rotate `session_id`. Acceptable cost; the false-409 fix is a correctness improvement, and benchmark sessions are short-lived. | +| **U5 TypedDict reveals shape drift the current codebase tolerates.** One construction site might omit a field that consumers expect. | The plan's verification step requires existing tests to stay green; a runtime AttributeError would fail `tests.test_e2e_phase1`. Use `Required[...]` for fields all sites set today; downgrade to `NotRequired` only after evidence. | +| **U2 envelope normalization is observable to any HTTP client reading `response.json()["error"]`.** | Internal grep shows no consumer reads `error`-key responses from bench-collections; if one is found in implementation, document and convert it. | +| **U4 helper change accidentally regresses content hydration on records outside the override map.** | Existing test_04d/04e assertions on non-empty `content` cover the regression. | +| **U1 410 route accidentally shadows the admin route.** | The legacy URL `/api/v1/benchmark/conversation_ingest` and the admin URL `/api/v1/admin/benchmark/conversation_ingest` have different prefixes; route matching is path-exact in FastAPI. Add a regression test that the admin URL still returns 200. | + +--- + +## Documentation / Operational Notes + +- **CHANGELOG entry** (created or updated by U1): note the URL move with deprecation date, the 410 shim removal target, and the U6 hash-canonicalization side effect that may produce one-time 409s for sessions ingested before this release. +- **No infrastructure changes.** No new env vars, no migration scripts, no deployment ordering constraints. + +--- + +## Sources & References + +- **Prior REVIEW.md:** `.context/compound-engineering/ce-code-review/20260424-152926-6301c860/REVIEW.md` +- **Follow-up REVIEW.md:** `.context/compound-engineering/ce-code-review/20260425-093102-b5a311de/REVIEW.md` +- **Residual handoff:** `docs/residual-review-findings/feat-benchmark-offline-conv-ingest.md` +- **Phase 1 plan (template followed by this plan):** `docs/plans/2026-04-25-003-fix-benchmark-conversation-ingest-review-fixes-plan.md` +- **Related PRs:** #3 (Phase 1 + must-address residuals), #4 (should-address residuals) +- **Branch base:** `master @ 90eb069` (after PR #4 merge) diff --git a/src/opencortex/context/manager.py b/src/opencortex/context/manager.py index 635952c..eef0997 100644 --- a/src/opencortex/context/manager.py +++ b/src/opencortex/context/manager.py @@ -31,6 +31,7 @@ set_request_identity, set_request_project_id, ) +from opencortex.context.recomposition_types import RecompositionEntry 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 @@ -1146,14 +1147,50 @@ async def _persist_conversation_source( transcript=transcript, ) + @staticmethod + def _canonicalize_for_hash(value: Any) -> Any: + """Recursively canonicalize a value so benign reordering does not + change the digest (REVIEW ADV-006). + + - ``dict`` values: recurse on each entry. Key ordering is handled + downstream by ``OPT_SORT_KEYS`` during serialization. + - ``list`` values: when every element is a primitive + (``str | int | float | bool | None``), sort the list — these + are typically anchor sets like ``time_refs`` whose order is + not semantic. When elements are dicts (e.g. ``tool_calls``), + leave order intact: the sequence carries meaning. + - Everything else: returned as-is. Strings, numbers, None, and + any non-list/dict objects pass through unchanged. + """ + if isinstance(value, dict): + return {k: ContextManager._canonicalize_for_hash(v) for k, v in value.items()} + if isinstance(value, list): + if all(isinstance(item, (str, int, float, bool, type(None))) for item in value): + # Sort by string projection to keep mixed-type lists stable; + # primitive lists in benchmark meta are almost always + # homogeneous strings. + return sorted(value, key=lambda x: (x is None, str(x))) + return [ContextManager._canonicalize_for_hash(item) for item in value] + return value + @staticmethod def _hash_transcript(transcript: List[Dict[str, Any]]) -> str: - """SHA-256 over the canonical normalized transcript shape.""" + """SHA-256 over the canonical normalized transcript shape. + + Message order is semantic and preserved as-is. Inside each + message's ``meta`` dict, list values that contain only + primitives are sorted so benign reordering of e.g. ``time_refs`` + does not produce a false 409 conflict on benchmark replay + (REVIEW ADV-006). Lists of dicts (``tool_calls``) keep their + sequence — order is treated as semantic for those. + """ normalized = [ { "role": str(message.get("role", "") or ""), "content": str(message.get("content", "") or ""), - "meta": message.get("meta") or {}, + "meta": ContextManager._canonicalize_for_hash( + message.get("meta") or {} + ), } for message in transcript ] @@ -1436,19 +1473,24 @@ def _export_memory_record( async def _hydrate_record_contents( self, records: List[Dict[str, Any]], + overrides: Optional[Dict[str, str]] = None, ) -> Dict[str, str]: - """Batch-read L2 content for a record set; missing files map to ''. - - Used by the benchmark response builder so adapters can fall back - on raw conversation text when L0/L1 are short. Sequential or - gather-based read is fine here: per-conversation set is dozens of - records, not thousands. Failures are logged once per URI and the - record falls back to an empty string rather than failing the - whole ingest. + """Return URI -> L2 content for a record set. + + ``overrides`` short-circuits the FS read for URIs already in + memory (REVIEW KP-06). The benchmark merged_recompose path + passes the in-memory write-time map so the response avoids + racing the orchestrator's fire-and-forget CortexFS write; the + direct_evidence path passes its own captured map. Records whose + URI is not in ``overrides`` go through the existing CortexFS + ``read_file(uri/content.md)`` path; missing files map to + empty string so a single FS hiccup does not fail the response. + + Returned dict contains every URI extracted from ``records`` so + the caller can do a flat ``hydrated.get(uri, "")`` instead of + layering 3 fallback dicts at the comprehension site. """ - fs = getattr(self._orchestrator, "_fs", None) - if fs is None: - return {} + overrides = overrides or {} uris = [ str(record.get("uri", "") or "").strip() for record in records @@ -1457,6 +1499,17 @@ async def _hydrate_record_contents( if not uris: return {} + result: Dict[str, str] = {uri: overrides[uri] for uri in uris if uri in overrides} + missing = [uri for uri in uris if uri not in overrides] + if not missing: + return result + + fs = getattr(self._orchestrator, "_fs", None) + if fs is None: + for uri in missing: + result[uri] = "" + return result + async def _read_one(uri: str) -> tuple[str, str]: try: return uri, await fs.read_file(f"{uri}/content.md") @@ -1468,15 +1521,16 @@ async def _read_one(uri: str) -> tuple[str, str]: ) return uri, "" - results = await asyncio.gather(*[_read_one(u) for u in uris]) - return dict(results) + fs_results = await asyncio.gather(*[_read_one(u) for u in missing]) + result.update(dict(fs_results)) + return result def _benchmark_recomposition_entries( self, normalized_segments: List[List[Dict[str, Any]]], - ) -> List[Dict[str, Any]]: + ) -> List[RecompositionEntry]: """Build message-level entries for benchmark offline chunking.""" - entries: List[Dict[str, Any]] = [] + entries: List[RecompositionEntry] = [] msg_index = 0 for segment in normalized_segments: segment_meta = self._benchmark_segment_meta(segment) @@ -1515,18 +1569,18 @@ def _benchmark_recomposition_entries( "entities": self._merge_unique_strings(meta.get("entities")), } entries.append( - { - "text": rendered, - "uri": "", - "msg_start": msg_index, - "msg_end": msg_index, - "token_count": max(self._estimate_tokens(rendered), 1), - "anchor_terms": self._segment_anchor_terms(record), - "time_refs": self._segment_time_refs(record), - "source_record": record, - "immediate_uris": [], - "superseded_merged_uris": [], - } + RecompositionEntry( + text=rendered, + uri="", + msg_start=msg_index, + msg_end=msg_index, + token_count=max(self._estimate_tokens(rendered), 1), + anchor_terms=self._segment_anchor_terms(record), + time_refs=self._segment_time_refs(record), + source_record=record, + immediate_uris=[], + superseded_merged_uris=[], + ) ) msg_index += 1 return entries @@ -1848,22 +1902,15 @@ async def _bounded_complete( # records by guessing session_ids (R2-04). Adapter doesn't # consume it; dropping is cheaper than scoping correctly. # - # ``_orchestrator.add`` writes content via fire-and-forget - # to CortexFS, so a FS-only readback here would race; we - # use the in-memory map captured during the write loop. - # FS read is the fallback for records that came back from - # ``_load_session_merged_records`` outside our write set - # (e.g. directory records added during recomposition). - fallback_targets = [ - record - for record in merged_records - if str(record.get("uri", "") or "") - not in merged_content_by_uri - ] - fallback_hydrated = ( - await self._hydrate_record_contents(fallback_targets) - if fallback_targets - else {} + # 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 @@ -1880,12 +1927,9 @@ async def _bounded_complete( "records": [ self._export_memory_record( record, - hydrated_content=merged_content_by_uri.get( + hydrated_content=hydrated.get( str(record.get("uri", "") or ""), - fallback_hydrated.get( - str(record.get("uri", "") or ""), - "", - ), + "", ), ) for record in merged_records @@ -2007,6 +2051,14 @@ async def _benchmark_ingest_direct_evidence( # 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, @@ -2016,7 +2068,7 @@ async def _benchmark_ingest_direct_evidence( "records": [ self._export_memory_record( record, - hydrated_content=evidence_content_by_uri.get( + hydrated_content=hydrated.get( str(record.get("uri", "") or ""), "", ), @@ -2333,9 +2385,9 @@ async def _build_recomposition_entries( snapshot: ConversationBuffer, immediate_records: List[Dict[str, Any]], tail_records: List[Dict[str, Any]], - ) -> List[Dict[str, Any]]: + ) -> List[RecompositionEntry]: """Build ordered recomposition entries from merged-tail + immediates.""" - entries: List[Dict[str, Any]] = [] + entries: List[RecompositionEntry] = [] fs = getattr(self._orchestrator, "_fs", None) tail_uris = [ @@ -2363,18 +2415,18 @@ async def _read_l2(uri: str) -> str: if not text: continue entries.append( - { - "text": text, - "uri": uri, - "msg_start": msg_range[0], - "msg_end": msg_range[1], - "token_count": max(self._estimate_tokens(text), 1), - "anchor_terms": self._segment_anchor_terms(record), - "time_refs": self._segment_time_refs(record), - "source_record": record, - "immediate_uris": [], - "superseded_merged_uris": ([uri] if uri else []), - } + RecompositionEntry( + text=text, + uri=uri, + msg_start=msg_range[0], + msg_end=msg_range[1], + token_count=max(self._estimate_tokens(text), 1), + anchor_terms=self._segment_anchor_terms(record), + time_refs=self._segment_time_refs(record), + source_record=record, + immediate_uris=[], + superseded_merged_uris=([uri] if uri else []), + ) ) by_uri = { @@ -2404,18 +2456,18 @@ async def _read_l2(uri: str) -> str: msg_index = snapshot.start_msg_index + offset msg_range = (msg_index, msg_index) entries.append( - { - "text": str(text), - "uri": normalized_uri, - "msg_start": msg_range[0], - "msg_end": msg_range[1], - "token_count": max(self._estimate_tokens(text), 1), - "anchor_terms": self._segment_anchor_terms(record), - "time_refs": self._segment_time_refs(record), - "source_record": record, - "immediate_uris": ([normalized_uri] if normalized_uri else []), - "superseded_merged_uris": [], - } + RecompositionEntry( + text=str(text), + uri=normalized_uri, + msg_start=msg_range[0], + msg_end=msg_range[1], + token_count=max(self._estimate_tokens(text), 1), + anchor_terms=self._segment_anchor_terms(record), + time_refs=self._segment_time_refs(record), + source_record=record, + immediate_uris=([normalized_uri] if normalized_uri else []), + superseded_merged_uris=[], + ) ) entries.sort( @@ -2429,14 +2481,14 @@ async def _read_l2(uri: str) -> str: def _build_recomposition_segments( self, - entries: List[Dict[str, Any]], + entries: List[RecompositionEntry], ) -> List[Dict[str, Any]]: """Split ordered recomposition entries into bounded semantic segments.""" if not entries: return [] segments: List[Dict[str, Any]] = [] - current: List[Dict[str, Any]] = [] + current: List[RecompositionEntry] = [] current_tokens = 0 current_messages = 0 @@ -2479,7 +2531,7 @@ def _build_recomposition_segments( def _build_anchor_clustered_segments( self, - entries: List[Dict[str, Any]], + entries: List[RecompositionEntry], ) -> List[Dict[str, Any]]: """Cluster entries by anchor Jaccard similarity for full_recompose.""" if not entries: @@ -2495,7 +2547,7 @@ def _build_anchor_clustered_segments( # APPEND, never SPLIT. Now an oversized seed flushes immediately # as its own single-entry segment and the next entry becomes the # new seed. - current: List[Dict[str, Any]] = [] + current: List[RecompositionEntry] = [] current_anchors: Set[str] = set() current_tokens = 0 current_messages = 0 @@ -2507,7 +2559,7 @@ def _within_caps_with(entry_tokens: int, entry_messages: int) -> bool: <= _RECOMPOSE_CLUSTER_MAX_MESSAGES ) - def _seed_with(entry: Dict[str, Any]) -> None: + def _seed_with(entry: RecompositionEntry) -> None: nonlocal current, current_anchors, current_tokens, current_messages entry_msgs = int(entry["msg_end"]) - int(entry["msg_start"]) + 1 current = [entry] @@ -2582,7 +2634,7 @@ def _seed_with(entry: Dict[str, Any]) -> None: def _finalize_recomposition_segment( self, - entries: List[Dict[str, Any]], + entries: List[RecompositionEntry], ) -> Dict[str, Any]: """Materialize one recomposition segment payload.""" msg_starts = [int(entry["msg_start"]) for entry in entries] @@ -2933,7 +2985,7 @@ async def _run_full_session_recomposition( if len(merged_records) <= 1: return [] if return_created_uris else None - entries: List[Dict[str, Any]] = [] + entries: List[RecompositionEntry] = [] for record in merged_records: msg_range = self._record_msg_range(record) if msg_range is None: @@ -2943,18 +2995,18 @@ async def _run_full_session_recomposition( if not text: continue entries.append( - { - "text": text, - "uri": uri, - "msg_start": msg_range[0], - "msg_end": msg_range[1], - "token_count": max(self._estimate_tokens(text), 1), - "anchor_terms": self._segment_anchor_terms(record), - "time_refs": self._segment_time_refs(record), - "source_record": record, - "immediate_uris": [], - "superseded_merged_uris": [], - } + RecompositionEntry( + text=text, + uri=uri, + msg_start=msg_range[0], + msg_end=msg_range[1], + token_count=max(self._estimate_tokens(text), 1), + anchor_terms=self._segment_anchor_terms(record), + time_refs=self._segment_time_refs(record), + source_record=record, + immediate_uris=[], + superseded_merged_uris=[], + ) ) segments = self._build_anchor_clustered_segments(entries) diff --git a/src/opencortex/context/recomposition_types.py b/src/opencortex/context/recomposition_types.py new file mode 100644 index 0000000..e8b3868 --- /dev/null +++ b/src/opencortex/context/recomposition_types.py @@ -0,0 +1,47 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Typed shape for entries flowing through anchor-clustered recomposition. + +Defined in its own module so ``ContextManager`` and consumers can import +the type without pulling each other into a circular import. The shape +is enforced at construction time across three sites in ``manager.py``: + +- ``ContextManager._benchmark_recomposition_entries`` — message-level + entries from the benchmark offline ingest path. +- ``ContextManager._build_recomposition_entries`` — production + conversation lifecycle entries. +- The inline builder inside ``ContextManager._run_full_session_recomposition`` + that re-derives entries from already-stored merged records. + +Keeping a single ``RecompositionEntry`` TypedDict instead of bare +``Dict[str, Any]`` lets consumers (``_build_anchor_clustered_segments``, +``_finalize_recomposition_segment``) annotate against a stable shape. +Drift across the three construction sites surfaces as a type error +the next time mypy / pyright runs, instead of as a silent runtime gap. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Set, TypedDict + + +class RecompositionEntry(TypedDict): + """Single entry consumed by anchor-clustered recomposition. + + Every field is set unconditionally at all three construction sites + today. ``immediate_uris`` and ``superseded_merged_uris`` are + populated for production conversation entries and stay as empty + lists for benchmark and re-derived entries — the lists are + structurally required even when empty so downstream consumers + never need to ``.get`` them with a default. + """ + + text: str + uri: str + msg_start: int + msg_end: int + token_count: int + anchor_terms: Set[str] + time_refs: Set[str] + source_record: Dict[str, Any] + immediate_uris: List[str] + superseded_merged_uris: List[str] diff --git a/src/opencortex/http/admin_routes.py b/src/opencortex/http/admin_routes.py index 95572c4..20609f3 100644 --- a/src/opencortex/http/admin_routes.py +++ b/src/opencortex/http/admin_routes.py @@ -12,7 +12,6 @@ from typing import Any, Dict, Optional from fastapi import APIRouter, HTTPException, Request -from fastapi.responses import JSONResponse from opencortex.auth.token import ( generate_token, load_token_records, revoke_token, save_token_record, @@ -208,7 +207,14 @@ async def create_bench_collection(request: Request): body = await request.json() name = body.get("name", "") if not name.startswith("bench_"): - return JSONResponse({"error": "Collection name must start with bench_"}, status_code=400) + # REVIEW api-contract-004: standardize on FastAPI's + # ``{"detail": ...}`` envelope across every admin route. The + # prior ``JSONResponse({"error": ...})`` was the only third + # style in this file. + raise HTTPException( + status_code=400, + detail="Collection name must start with bench_", + ) dim = _orchestrator._config.embedding_dimension from opencortex.storage.collection_schemas import CollectionSchemas schema = CollectionSchemas.context_collection(name, dim) @@ -221,7 +227,10 @@ async def delete_bench_collection(name: str): """Delete a benchmark-isolated collection (name must start with bench_).""" _require_admin() if not name.startswith("bench_"): - return JSONResponse({"error": "Can only delete bench_ collections"}, status_code=400) + raise HTTPException( + status_code=400, + detail="Can only delete bench_ collections", + ) await _orchestrator._storage.drop_collection(name) return {"status": "deleted", "collection": name} diff --git a/src/opencortex/http/models.py b/src/opencortex/http/models.py index bb1e046..e2712f0 100644 --- a/src/opencortex/http/models.py +++ b/src/opencortex/http/models.py @@ -5,9 +5,9 @@ ``mcp_server.py``. """ -import json from typing import Any, Dict, List, Optional +import orjson from pydantic import BaseModel, Field, field_validator # ========================================================================= @@ -320,11 +320,15 @@ def _meta_within_byte_budget( ) -> Optional[Dict[str, Any]]: if value is None: return value + # Use orjson per project convention (REVIEW KP-01). orjson + # always emits UTF-8 bytes — no ``ensure_ascii`` flag needed, + # and the returned bytes are exactly what we want to count + # against the byte budget without a separate ``.encode``. try: - serialized = json.dumps(value, ensure_ascii=False, sort_keys=True) - except (TypeError, ValueError) as exc: + serialized = orjson.dumps(value, option=orjson.OPT_SORT_KEYS) + except (TypeError, orjson.JSONEncodeError) as exc: raise ValueError("meta must be JSON-serializable") from exc - if len(serialized.encode("utf-8")) > _BENCHMARK_MAX_META_BYTES: + if len(serialized) > _BENCHMARK_MAX_META_BYTES: raise ValueError( f"meta exceeds {_BENCHMARK_MAX_META_BYTES}-byte limit" ) diff --git a/src/opencortex/http/server.py b/src/opencortex/http/server.py index ddf7400..2611948 100644 --- a/src/opencortex/http/server.py +++ b/src/opencortex/http/server.py @@ -323,6 +323,39 @@ def create_app() -> FastAPI: def _register_routes(app: FastAPI) -> None: """Register all REST endpoints on *app*.""" + # ===================================================================== + # Deprecation shims + # ===================================================================== + + @app.api_route( + "/api/v1/benchmark/conversation_ingest", + methods=["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"], + include_in_schema=False, + ) + async def _legacy_benchmark_conversation_ingest_gone() -> Dict[str, Any]: + """Return 410 Gone for the pre-admin-gate URL. + + The benchmark ingest endpoint moved under the admin namespace in + v0.7.x to add ``_require_admin()`` enforcement (see + ``CHANGELOG.md``). This shim makes the move discoverable for any + out-of-tree caller still pointing at the old path; FastAPI's + default 404 carries no migration breadcrumb. Drop after one or + two releases of grace. + """ + raise HTTPException( + status_code=410, + detail={ + "reason": "moved", + "new_url": "/api/v1/admin/benchmark/conversation_ingest", + "removed_in": "0.8.0", + "note": ( + "Endpoint relocated under the admin namespace and now " + "requires admin role. Use the new URL with an admin " + "Bearer token." + ), + }, + ) + # ===================================================================== # Core Memory # ===================================================================== diff --git a/tests/test_benchmark_ingest_lifecycle.py b/tests/test_benchmark_ingest_lifecycle.py index 32776a0..df9e2fe 100644 --- a/tests/test_benchmark_ingest_lifecycle.py +++ b/tests/test_benchmark_ingest_lifecycle.py @@ -140,6 +140,81 @@ async def check(): self._run(check()) + def test_hash_transcript_canonicalizes_meta_list_ordering(self): + """Reordering primitive lists inside meta does not change hash. + + Two transcripts that differ only in the ordering of `time_refs` + list elements must produce the same SHA-256 digest, so benign + benchmark replays do not get false HTTP 409 conflicts (REVIEW + ADV-006). Lists of dicts (e.g. tool_calls) keep their order — + sequence is semantic for those. + """ + from opencortex.context.manager import ContextManager + + a = [ + { + "role": "user", + "content": "hi", + "meta": { + "time_refs": ["2023-05-01", "9 am on 1 May"], + "entities": ["Alice", "Bob"], + }, + } + ] + b = [ + { + "role": "user", + "content": "hi", + "meta": { + "time_refs": ["9 am on 1 May", "2023-05-01"], + "entities": ["Bob", "Alice"], + }, + } + ] + self.assertEqual( + ContextManager._hash_transcript(a), + ContextManager._hash_transcript(b), + "primitive-list reordering inside meta must not change hash", + ) + + # Genuine content difference still changes hash. + c = [ + { + "role": "user", + "content": "different message", + "meta": {"time_refs": ["2023-05-01"]}, + } + ] + self.assertNotEqual( + ContextManager._hash_transcript(a), + ContextManager._hash_transcript(c), + ) + + # tool_calls (list of dicts) — order IS semantic, hash differs. + d = [ + { + "role": "user", + "content": "hi", + "meta": { + "tool_calls": [{"name": "a"}, {"name": "b"}], + }, + } + ] + e = [ + { + "role": "user", + "content": "hi", + "meta": { + "tool_calls": [{"name": "b"}, {"name": "a"}], + }, + } + ] + self.assertNotEqual( + ContextManager._hash_transcript(d), + ContextManager._hash_transcript(e), + "tool_calls order is semantic; reordering must change hash", + ) + def test_torn_prior_run_is_not_treated_as_idempotent(self): """Hash-match without run_complete marker triggers purge + re-ingest. diff --git a/tests/test_http_server.py b/tests/test_http_server.py index 2133fec..93950d9 100644 --- a/tests/test_http_server.py +++ b/tests/test_http_server.py @@ -1026,6 +1026,108 @@ async def check(): self._run(check()) + def test_04i_admin_collection_uses_detail_envelope(self): + """Admin collection routes use {"detail": ...} on 400, matching admin convention.""" + + async def check(): + from opencortex.http.request_context import ( + reset_request_role, + set_request_role, + ) + + async with _test_app_context() as client: + role_token = set_request_role("admin") + try: + # Bad name → 400 with detail envelope (was {"error": ...} before). + resp = await client.post( + "/api/v1/admin/collection", + json={"name": "not_a_bench_prefix"}, + ) + self.assertEqual(resp.status_code, 400) + body = resp.json() + self.assertIn("detail", body) + self.assertNotIn("error", body) + self.assertIn("bench_", str(body["detail"])) + + # DELETE with non-bench name follows same shape. + resp_d = await client.delete( + "/api/v1/admin/collection/not_a_bench_prefix" + ) + self.assertEqual(resp_d.status_code, 400) + body_d = resp_d.json() + self.assertIn("detail", body_d) + self.assertNotIn("error", body_d) + finally: + reset_request_role(role_token) + + self._run(check()) + + def test_04h_legacy_benchmark_url_returns_410_gone(self): + """Legacy URL `/api/v1/benchmark/conversation_ingest` returns 410. + + The endpoint moved under /api/v1/admin/ in v0.7.x; the shim + keeps a discoverable migration breadcrumb for out-of-tree + callers (REVIEW api-contract-001). + """ + + async def check(): + async with _test_app_context() as client: + # POST is the original verb the endpoint accepted. + resp = await client.post( + "/api/v1/benchmark/conversation_ingest", + json={"session_id": "x", "segments": []}, + ) + self.assertEqual(resp.status_code, 410) + detail = resp.json().get("detail", {}) + self.assertEqual(detail.get("reason"), "moved") + self.assertEqual( + detail.get("new_url"), + "/api/v1/admin/benchmark/conversation_ingest", + ) + + # Other verbs hit the same shim — useful for clients + # that probe with HEAD/OPTIONS before POSTing. + for method in ("get", "put", "patch", "delete"): + resp_v = await getattr(client, method)( + "/api/v1/benchmark/conversation_ingest" + ) + self.assertEqual( + resp_v.status_code, + 410, + f"{method.upper()} on legacy URL must also 410", + ) + + # New admin URL still works (regression lock so the shim + # does not accidentally shadow the live endpoint). + from opencortex.http.request_context import ( + reset_request_role, + set_request_role, + ) + + role_token = set_request_role("admin") + try: + live = await client.post( + "/api/v1/admin/benchmark/conversation_ingest", + json={ + "session_id": "bench_legacy_lock", + "segments": [ + { + "messages": [ + { + "role": "user", + "content": "ok", + } + ] + } + ], + }, + ) + self.assertEqual(live.status_code, 200) + finally: + reset_request_role(role_token) + + self._run(check()) + def test_04f_benchmark_ingest_requires_admin(self): """Non-admin token receives 403 from benchmark ingest endpoint.""" @@ -1119,6 +1221,54 @@ async def check(): json=too_big_meta, ) self.assertEqual(resp.status_code, 422) + + # CJK content near the meta byte budget — orjson + # always emits UTF-8 so the byte count is the real + # ceiling, not a Python-string-length proxy + # (REVIEW KP-01 regression). + cjk_value = "你" * 5_000 # ~15 KB UTF-8, under 16 KB + cjk_meta_ok = { + "session_id": "bench_cjk_ok", + "segments": [ + { + "messages": [ + { + "role": "user", + "content": "ok", + "meta": {"k": cjk_value}, + } + ] + } + ], + } + resp = await client.post( + "/api/v1/admin/benchmark/conversation_ingest", + json=cjk_meta_ok, + ) + # validates (200 success path; happy path body + # detail not in scope here, only schema acceptance). + self.assertEqual(resp.status_code, 200) + + cjk_value_over = "你" * 6_000 # ~18 KB UTF-8 + cjk_meta_over = { + "session_id": "bench_cjk_over", + "segments": [ + { + "messages": [ + { + "role": "user", + "content": "ok", + "meta": {"k": cjk_value_over}, + } + ] + } + ], + } + resp = await client.post( + "/api/v1/admin/benchmark/conversation_ingest", + json=cjk_meta_over, + ) + self.assertEqual(resp.status_code, 422) finally: reset_request_role(role_token) From 2c6a388b72cf4e2c4c73af8a196b677a861afbad Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 11:01:37 +0800 Subject: [PATCH 2/3] fix(review): apply autofix feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply 2 safe_auto fixes from CE code review (run 20260425-105317-823f3ead). 99/99 tests pass. - KP-09: drop redundant `orjson.JSONEncodeError` from the meta validator's except tuple — it IS `TypeError` (verified at runtime, same class object), so the bare TypeError catch already covers it. Added an inline note so future maintainers do not re-introduce the redundant clause. - KP-13: `tuple[str, str]` → `Tuple[str, str]` in `_hydrate_record_contents._read_one` for file-wide consistency (manager.py imports and uses `typing.Tuple` everywhere else). This was REVIEW KP-02 from earlier rounds, now adjacent to changed code. Residual P3 nice-to-haves (testing gaps, style nits, the knowledge/archivist envelope drift left out of the plan's scope) are captured in the run artifact for follow-up: .context/compound-engineering/ce-code-review/20260425-105317-823f3ead/REVIEW.md --- src/opencortex/context/manager.py | 2 +- src/opencortex/http/models.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/opencortex/context/manager.py b/src/opencortex/context/manager.py index eef0997..20afc70 100644 --- a/src/opencortex/context/manager.py +++ b/src/opencortex/context/manager.py @@ -1510,7 +1510,7 @@ async def _hydrate_record_contents( result[uri] = "" return result - async def _read_one(uri: str) -> tuple[str, str]: + async def _read_one(uri: str) -> Tuple[str, str]: try: return uri, await fs.read_file(f"{uri}/content.md") except Exception as exc: # pragma: no cover - defensive diff --git a/src/opencortex/http/models.py b/src/opencortex/http/models.py index e2712f0..1d37a46 100644 --- a/src/opencortex/http/models.py +++ b/src/opencortex/http/models.py @@ -324,9 +324,12 @@ def _meta_within_byte_budget( # always emits UTF-8 bytes — no ``ensure_ascii`` flag needed, # and the returned bytes are exactly what we want to count # against the byte budget without a separate ``.encode``. + # ``orjson.JSONEncodeError`` is literally ``TypeError`` so the + # plain TypeError catch covers both encoder failures and + # non-serializable values. try: serialized = orjson.dumps(value, option=orjson.OPT_SORT_KEYS) - except (TypeError, orjson.JSONEncodeError) as exc: + except TypeError as exc: raise ValueError("meta must be JSON-serializable") from exc if len(serialized) > _BENCHMARK_MAX_META_BYTES: raise ValueError( From 2f60e79e984860ec16fa9d1c89cb0b29e62fa3db Mon Sep 17 00:00:00 2001 From: Hugo <690836098@qq.com> Date: Sat, 25 Apr 2026 11:03:45 +0800 Subject: [PATCH 3/3] docs(review): record residual review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Durable handoff for residuals from CE code review run 20260425-105317-823f3ead. No GitHub PR existed at the time of the review, so per LFG step 6 the residuals land in this fallback file. When a PR is opened these items should be copied into the PR body and this file deleted. The review verdict was Ready to merge — all residuals are advisory, nice-to-have testing/style items, or learning-capture candidates. The single concrete should-consider item is api-contract-004-residual (knowledge/archivist routes still use the {"error":...} envelope; the JS consumer at web/src/api/client.ts:82-84 has a fragile special case for it). Plan scope was bench-collections only so this stays out of the current PR. --- .../fix-benchmark-ingest-p2-residuals.md | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 docs/residual-review-findings/fix-benchmark-ingest-p2-residuals.md diff --git a/docs/residual-review-findings/fix-benchmark-ingest-p2-residuals.md b/docs/residual-review-findings/fix-benchmark-ingest-p2-residuals.md new file mode 100644 index 0000000..15f057d --- /dev/null +++ b/docs/residual-review-findings/fix-benchmark-ingest-p2-residuals.md @@ -0,0 +1,71 @@ +# Residual Review Findings — fix/benchmark-ingest-p2-residuals + +**Source:** ce-code-review run `20260425-105317-823f3ead` +**Mode:** autofix (LFG pipeline) +**Plan:** `docs/plans/2026-04-25-004-refactor-benchmark-ingest-p2-residuals-plan.md` +**Verdict:** Ready to merge +**Branch HEAD:** `2c6a388` +**Run artifact:** `.context/compound-engineering/ce-code-review/20260425-105317-823f3ead/REVIEW.md` + +This file is the durable handoff for residuals because no GitHub PR existed at the time of the review. When a PR is opened, copy these items into the PR body and delete this file. + +--- + +## Residual Review Findings + +### Should consider before merging this PR + +- **[P3][manual] `src/opencortex/http/server.py:589-627` — api-contract-004-residual: knowledge/archivist routes still return `{"error": ...}`** + Plan R2's scope was bench-collections only, so this is technically out of scope for this PR. But the same envelope drift exists in knowledge/archivist routes, and `web/src/api/client.ts:82-84` has a fragile special case parsing the `error` key for those routes. If you want to extend the envelope unification beyond bench-collections, do it in a small follow-up that touches both the route AND the JS consumer at the same time — don't leave the JS half-converted. + *Suggested fix:* convert the routes to `HTTPException(detail=...)`, drop the `error` parsing in `web/src/api/client.ts`, run the frontend regression suite. + +### Defer to a follow-up polish PR (small testing + style) + +- **[P3][manual] `tests/test_http_server.py:test_04h` — T-001: HEAD verb missing from multi-verb shim test.** The 410 shim explicitly registers OPTIONS and FastAPI auto-generates HEAD; the test loop covers GET/PUT/PATCH/DELETE only. Add HEAD + OPTIONS to the loop to lock the contract. + +- **[P3][manual] `tests/` — T-002: `_hydrate_record_contents(overrides=...)` branch coverage gap.** Existing test_04d/04e cover end-to-end content correctness; the override short-circuit / fs-None / mixed paths are not directly exercised. Add a unit test that asserts no FS call happens when every URI is in `overrides`. + +- **[P3][manual] `tests/test_http_server.py:test_04i` — T-003: envelope lock incomplete.** The test asserts `"detail" in body` and `"error" not in body` only for the 400 paths; 409 (dict detail) and 504 envelopes have no equivalent regression assertion. Add the same lock to test_04d (409 hash conflict) and test_15 (504 timeout — if exists, otherwise add). + +- **[P3][manual] `tests/` — T-005: legacy-hash migration scenario untested.** CHANGELOG documents a one-time 409 on pre-existing source records ingested before U6's hash canonicalization. No test simulates this — to lock it down, write a record with a stale-format hash directly into storage, replay, assert 409 with both hashes in the detail. + +- **[P3][manual] `src/opencortex/context/recomposition_types.py` — T-006: TypedDict is documentation-only at runtime.** No mypy / pyright in CI. The drift-detection value of the new TypedDict is theoretical until a type-check pass lands. Either add `mypy --strict src/opencortex/context/manager.py` to the test pipeline or accept the limitation. + +- **[P3][manual] `src/opencortex/context/manager.py` consumers of RecompositionEntry — KP-10: defensive casting still present.** Consumers wrap reads in `int(entry["msg_start"])` / `str(entry["uri"])` even though the TypedDict guarantees the types. Drop the casts to actually rely on the type contract. + +- **[P3][manual] `src/opencortex/context/recomposition_types.py` — KP-11: consider `@dataclass(frozen=True, slots=True)` instead of TypedDict.** Would give immutability + free `__repr__`/equality + same module-isolation benefit. TypedDict is defensible; dataclass would be stronger if you want runtime help, not just typing. + +- **[P3][manual] `src/opencortex/context/manager.py:1170` — KP-12: `_canonicalize_for_hash` sort key over-engineered.** Sort key `(x is None, str(x))` defends against mixed-type lists that the inline comment says don't really occur in practice. Could simplify to `sorted(value, key=str)`. + +- **[P3][manual] `src/opencortex/context/manager.py:1492` — KP-14: `overrides = overrides or {}` rebinds parameter.** Minor style; rebind to a new local for readability. + +- **[P3][manual] `tests/test_http_server.py:1029` — ps-001: test method file ordering.** New `test_04i` is before `test_04h`, and both before pre-existing `test_04g`. Cosmetic; unittest doesn't depend on order. Reorder to match file-wide alphabetic suffix convention. + +- **[P3][advisory] `src/opencortex/context/manager.py` + `src/opencortex/http/admin_routes.py` — M-RR1: inline `(REVIEW …)` tags pattern continues to grow.** This PR adds 7 more tags; repo-wide src/ count now 21. Strip-at-merge convention or move tags to git trailers in a future polish PR. + +- **[P3][manual] `CHANGELOG.md` — M-RR4: no documented promotion ritual.** New file with no prior repo convention; relationship to `MEMORY.md` and the release-cycle ritual ("who promotes Unreleased?") undocumented. Add a brief CONTRIBUTING.md note. + +- **[P3][manual] `src/opencortex/http/server.py:328-358` — M-RR5: 410 shim needs grep-able TODO marker.** CHANGELOG says removal in v0.8.0; add a `# TODO(v0.8.0): remove this shim` so the eventual cleanup is automatable rather than prose-driven. + +--- + +## Applied This Run (autofix) + +| Finding | File | Change | +|---|---|---| +| KP-09 | `src/opencortex/http/models.py` | Drop `orjson.JSONEncodeError` from except tuple — it IS `TypeError`. Inline note added. | +| KP-13 | `src/opencortex/context/manager.py` | `tuple[str, str]` → `Tuple[str, str]` for file-wide consistency. | + +Committed as `2c6a388 fix(review): apply autofix feedback`. 99/99 tests pass. + +--- + +## Capture After Merge (institutional learnings) + +The learnings researcher flagged 5 strong `/ce-compound` candidates with no existing coverage — net-new institutional territory: + +1. **FastAPI error envelope convention.** Document the chosen shape (`{"detail": ...}` standard, `{"error": ...}` legacy-only). Cover both string and dict `detail` cases. +2. **410 Gone deprecation shim pattern.** Status, body shape, CHANGELOG breadcrumb, expiry date, `# TODO(version)` marker. +3. **orjson migration completeness rule.** Gotchas: bytes return, `OPT_SORT_KEYS` flag (no `sort_keys=True` kwarg), no `default=str` shorthand for datetimes — use `OPT_NAIVE_UTC | OPT_SERIALIZE_NUMPY`. +4. **TypedDict for cross-call-site shape contracts.** When to promote `Dict[str, Any]` to TypedDict (rule of thumb: ≥3 construction sites + ≥1 consumer that reads named keys). The recomposition entries case is a worked example. +5. **Hash canonicalization for benign reorderings.** Any hash used for idempotency keying must canonicalize both dict keys AND list values where order is non-semantic (e.g. `time_refs`, `tags`). Note the trade-off: only sort lists whose semantics tolerate it (preserve order for `messages`, `tool_calls`).