fix(benchmark): close 6 P2 residuals from prior CE review - #5
Merged
Conversation
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.
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
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the 6 P2/P3 residuals from the prior CE review tail that PRs #3 and #4 left for a follow-up polish PR. Source:
docs/residual-review-findings/feat-benchmark-offline-conv-ingest.mdand the run artifact at.context/compound-engineering/ce-code-review/20260425-105317-823f3ead/REVIEW.md.Plan:
docs/plans/2026-04-25-004-refactor-benchmark-ingest-p2-residuals-plan.mdWhat changed
/api/v1/benchmark/conversation_ingestreturns 410 Gone withdetail.new_url(was bare 404). NewCHANGELOG.mddocuments the URL move + the U6 hash side effect.src/opencortex/http/server.py,CHANGELOG.md{"detail": ...}envelope (was{"error": ...}).src/opencortex/http/admin_routes.pyBenchmarkConversationMessage.metavalidator usesorjson.dumpsinstead of stdlibjson.dumps— orjson always emits UTF-8 so the byte-budget check is exact.src/opencortex/http/models.py_hydrate_record_contents(records, overrides=None)collapses the merged_recompose / direct_evidence response builders' 3-level fallback chain into a singlehydrated.get(uri, "").src/opencortex/context/manager.pyRecompositionEntryTypedDict insrc/opencortex/context/recomposition_types.py; 3 construction sites + 4 consumers annotated. Pure type-shape change.src/opencortex/context/recomposition_types.py,src/opencortex/context/manager.py_hash_transcriptcanonicalizes lists of primitives in meta values before hashing — same digest fortime_refs: ["a","b"]vs["b","a"]. Lists of dicts (e.g.tool_calls) keep their order.src/opencortex/context/manager.pyTest plan
tests.test_http_server,tests.test_locomo_bench,tests.test_e2e_phase1,tests.test_benchmark_ingest_lifecycle.test_04h_legacy_benchmark_url_returns_410_gone(multi-verb + admin URL regression lock).test_04i_admin_collection_uses_detail_envelope(envelope shape lock with negative"error" not in bodyassertion).test_04gextended with CJK byte-budget regressions for U3.test_hash_transcript_canonicalizes_meta_list_ordering(primitive-list reorder same hash, content diff hash differs, tool_calls order preserved)._build_recomposition_entriesshared withcontext_end.Migration note (important)
U6 changes hash semantics. Source records ingested before this release used the old hash algorithm. The first benchmark replay of any pre-existing session will hit a hash mismatch and receive HTTP 409. To recover: delete the source record (
opencortex://{tenant}/{user}/session/conversations/{session_id}/source) or rotate thesession_id. After that, replays follow the new canonical-form hash and remain idempotent. Documented inCHANGELOG.md.Code review
CE autofix run
20260425-105317-823f3ead— verdict Ready to merge. 8 reviewers, 0 P0/P1, 1 P2 (KP-13:tuple→Tupleconsistency, applied), 13 P3 (1 applied as KP-09: redundantorjson.JSONEncodeErrorsince it ISTypeError, 12 deferred).Residual Review Findings
The remaining items from the run artifact are advisory or out-of-scope nice-to-haves; full list lives in
docs/residual-review-findings/fix-benchmark-ingest-p2-residuals.md. Highlights:{"error": ...}(server.py:589-627);web/src/api/client.ts:82-84has a fragile special case for it. Out of this plan's scope; recommend a small follow-up that flips both the route AND the JS consumer in lockstep.overridesbranches, envelope lock on 409/504, legacy-hash migration test, mypy in CI for the new TypedDict).Sequence
This is the third (and final) PR closing the prior CE review on the benchmark offline ingest path:
After this lands, every P0 / P1 / in-scope P2 from the original review is closed.
🤖 Generated with Claude Code