Add C++ rag-eval -live and search snapshot retry - #128
Conversation
Desk eval talks to loopback :8084 with the same needles as Go. Decode rejects unknown fields, duplicate ids, and empty needles. Search retries once if generation or health moved mid-query.
There was a problem hiding this comment.
🟡 Changes recommended
The new rag-eval desk decoder and WinHTTP response reader have concrete validation/error-propagation bugs that can accept invalid inputs or silently treat partial reads as success.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR completes two remaining Alexandria/RAG behaviors in the C++ implementation: adding rag-eval.exe -live (desk needle eval against loopback :8084) and adding a “consistent snapshot” retry in rag-service search (mirroring Go’s searchConsistently) so a mid-query generation/health move triggers a single retry.
Changes:
- Implement
rag-eval -livewith loopback-only endpoint allowlist, desk query JSON decoding, and an offline--self-test. - Add one-retry search behavior in C++
rag-servicewhen health/generation changes between pre- and post-search snapshots. - Wire
rag-evalself-test + offline eval into CMake/CTest and update cpp_memory_store cut notes.
File summaries
| File | Description |
|---|---|
| godbrain_core/cpp_memory_store/src/rag_eval_main.cpp | Adds WinHTTP loopback client, desk eval decoding, -live/-desk/-endpoint/-strict/--self-test handling. |
| godbrain_core/cpp_memory_store/src/rag_engine.cpp | Adds snapshot comparison helpers and wraps /v1/search with a 1-retry loop when health/generation shifts mid-query. |
| godbrain_core/cpp_memory_store/README.md | Updates “cut” notes documenting rag-eval -live and search snapshot retry behavior. |
| godbrain_core/cpp_memory_store/CMakeLists.txt | Adds CTest entries for rag-eval --self-test and offline corpus eval. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| std::string out; | ||
| for (;;) { | ||
| DWORD avail = 0; | ||
| if (!WinHttpQueryDataAvailable(request, &avail) || avail == 0) break; | ||
| if (out.size() + avail > 64 * 1024) avail = static_cast<DWORD>(64 * 1024 - out.size()); | ||
| if (avail == 0) break; | ||
| std::string chunk(avail, '\0'); | ||
| DWORD got = 0; | ||
| if (!WinHttpReadData(request, chunk.data(), avail, &got) || got == 0) break; | ||
| out.append(chunk.data(), got); | ||
| if (out.size() >= 64 * 1024) break; | ||
| } | ||
| *resp = std::move(out); | ||
| WinHttpCloseHandle(request); | ||
| WinHttpCloseHandle(connect); | ||
| WinHttpCloseHandle(session); | ||
| return true; |
| double topkd = 0; | ||
| if (!json_number(root, "top_k", &topkd)) { | ||
| if (err) *err = "desk evaluation top_k is invalid"; | ||
| return false; | ||
| } | ||
| out->top_k = static_cast<int>(topkd); |
| DeskQuery dq; | ||
| json_string(q, "id", &dq.id); | ||
| json_string(q, "query", &dq.query); | ||
| json_string(q, "sector", &dq.sector); |
There was a problem hiding this comment.
[suggestion] decode_desk_eval ignores json_string failure for optional sector. A missing key correctly becomes empty, but a present non-string ("sector": 1) is also treated as omitted. Go’s typed DecodeDeskEvalFile rejects that file. Live eval would then POST /v1/search without a sector filter, so needles can hit the wrong sector and the report can pass.
Suggestion: If json_has(q, "sector") and json_string fails, reject the query (same “query is invalid” path as a type error). Keep omitted sector as empty.
| if (err) *err = "desk evaluation top_k is invalid"; | ||
| return false; | ||
| } | ||
| out->top_k = static_cast<int>(topkd); |
There was a problem hiding this comment.
[suggestion] top_k is read as a double and static_cast<int>’d. Go unmarshals into int and rejects a fractional JSON number (8.5). Here 8.5 becomes 8 and passes the 1–25 range, so a malformed desk file still runs.
Suggestion: Require topkd to be finite and equal to std::trunc(topkd) (and in range) before storing out->top_k.
| return false; | ||
| } | ||
|
|
||
| bool same_search_snapshot( |
There was a problem hiding this comment.
[suggestion] same_search_snapshot / valid_response_capability are the fail-closed core of this PR (discard stale/partial results, stop before a second search when the retry look is unready). They live in rag_engine.cpp’s anonymous namespace with no C++ test. Go covers this with TestSearchHandlerReturnsOnlyStableSnapshot, TestSearchHandlerRejectsPartialProjectionInterleaving, TestSearchHandlerRetriesCompletedProjectionWithoutReturningStaleResult, TestSearchHandlerRejectsRepeatedRebuildActivation, and TestSameSearchSnapshotRejectsMismatchedHybridIdentity. ctest (cpp_rag_eval_self / cpp_rag_eval_offline) never hits rag-service search.
Suggestion: Lift the snapshot helpers (or a --self-test path) so a before/after HealthSnap plus retrieval/hybrid/degradation can be asserted offline: matching snapshots pass, unready after fails, generation/count movement fails, hybrid-without-embedding capability fails.
| return api_error(400, "retrieval_mode must be auto, lexical, or hybrid"); | ||
| } | ||
|
|
||
| // One retry if generation/health moved mid-query (Go searchConsistently). |
There was a problem hiding this comment.
[suggestion] // One retry if generation/health moved mid-query (Go searchConsistently). restates the loop and names the Go port. Comments should say why a constraint exists, not narrate the change.
Suggestion: Delete it, or keep a one-line why (e.g. do not return hits if generation/counts moved before the after-health check).
Reject a non-string sector and a fractional top_k. Drop the Go-port narration on the search retry loop.
Goal
Finish the remaining C++ Alexandria doors that still lived only in Go:
rag-eval -liveand GosearchConsistently.Go under
godbrain_core/memory_store/stays as rollback. Root:8082Go/Rust routers stay experimental.What
rag-eval.exe -livequeries loopback:8084with desk needles (-desk,-strict,-endpoint). Origin is127.0.0.1:8084orlocalhost:8084only.--self-testis offline. CMake also runs the hybrid corpus eval.rag-servicesearch fills health before and after the query and retries once if generation/health moved (same fail-closed mapping as Go).Verify
ctest --test-dir build/cpp_memory_store -C Releaseprotocol + rag-eval self + offline: pass:8084after bounce: hits=12 misses=0 empty=0, searchcitation_status=available