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/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`). diff --git a/src/opencortex/context/manager.py b/src/opencortex/context/manager.py index 635952c..20afc70 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,7 +1499,18 @@ async def _hydrate_record_contents( if not uris: return {} - async def _read_one(uri: str) -> tuple[str, str]: + 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") except Exception as exc: # pragma: no cover - defensive @@ -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..1d37a46 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,18 @@ 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``. + # ``orjson.JSONEncodeError`` is literally ``TypeError`` so the + # plain TypeError catch covers both encoder failures and + # non-serializable values. 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 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)