test(graph-rag): multi-snapshot query prototype and gate evidence (ADR 0020 Phase 2) - #296
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change adds a retrieval-recall evaluation CLI and golden dataset. It also adds an authorized multi-snapshot PostgreSQL query prototype, deterministic synthetic fixtures, integration tests, benchmarks, and a Phase 2 handoff. ChangesRetrieval-recall evaluation
Authorized multi-snapshot query prototype
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant GoldenDataset
participant ObservationSet
participant score
participant ReportFile
CLI->>GoldenDataset: load golden JSON
CLI->>ObservationSet: load observation JSON
CLI->>score: validate and score inputs
score-->>CLI: return recall report
CLI->>ReportFile: write formatted JSON
sequenceDiagram
participant IntegrationTests
participant MultiSnapshotSyntheticDataset
participant AuthorizedMultiSnapshotQuery
participant PostgreSQL
IntegrationTests->>MultiSnapshotSyntheticDataset: load deterministic fixture
MultiSnapshotSyntheticDataset->>PostgreSQL: insert graph and vector projections
IntegrationTests->>AuthorizedMultiSnapshotQuery: execute authorized query
AuthorizedMultiSnapshotQuery->>PostgreSQL: run compound or per-space SQL
PostgreSQL-->>AuthorizedMultiSnapshotQuery: return candidates and EXPLAIN data
AuthorizedMultiSnapshotQuery-->>IntegrationTests: return merged results and timings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TegamiThis repository uses Tegami to manage releases. When your changes affect published packages, add a changelog file under Create a changelog → · Changelog format Release preview
This PR does not add changelog files. Pending changelogs from other branches are included in the preview above. Run Managed by Tegami. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evaluation/src/orgmemory_eval/retrieval_recall.py`:
- Around line 90-91: Enforce the report schema v1 contract in the retrieval
evaluation entry point by rejecting any top_k value other than 40 before
retrieval runs. Keep the existing recall@40 labels, fields, and gate aligned
with this fixed value, and add a test that explicitly passes a non-default top_k
and verifies it is rejected.
In
`@integrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQuery.java`:
- Around line 340-347: The helper in AuthorizedMultiSnapshotQuery currently
infers the result shape from rows.getFirst(), misclassifying empty candidate
results and requiring unchecked casts. Split it into typed queryRows(...) and
queryExplain(...) methods, have each caller invoke the method matching its known
query shape, and preserve the existing result data and timing values without
runtime type inspection.
- Around line 406-419: Replace string concatenation in scopesJson with
Jackson-based JSON generation, serializing every SnapshotScope field and
authorized asset ID through the JSON writer so quotes, backslashes, and other
special characters are escaped correctly. Preserve the existing array/object
schema and output values consumed by jsonb_to_recordset, and use the existing
Jackson dependency rather than introducing a custom escaping implementation.
- Around line 225-236: Update ROW_ORDER so its spaceId, candidateId, and
contributionId tie-breakers compare UUIDs via toString(), matching PostgreSQL’s
unsigned-byte ordering and the existing SCORE_ORDER behavior. Leave the
globalRank ordering and comparator structure unchanged.
- Around line 56-95: Enforce ACL snapshot consistency in both
visible_entity_contributions and visible_relations by matching each
contribution’s acl_generation to scope.acl_generation, then update validateRows
to compare the row’s evidenceAclGeneration rather than the request-derived
value. Replace the substring-based projection checks in QUERY_BODY with exact
comma-delimited token matching for VECTOR and GRAPH, preserving the fail-closed
authorization behavior.
- Around line 328-334: Update the timeout setup in AuthorizedMultiSnapshotQuery
so the session-level statement_timeout is always restored before the pooled
connection is released. Wrap the timeout-scoped query work in a finally block
that resets the setting, or use SET LOCAL within an explicit transaction;
preserve the existing int-based timeout handling and do not change it solely for
the OpenGrep false positive.
In
`@integrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQueryIntegrationTests.java`:
- Around line 176-191: The reported p95 is actually the maximum of five samples,
so align the metric naming and handoff with the implementation. In
AuthorizedMultiSnapshotQueryIntegrationTests, either increase REPETITIONS to
support a meaningful p95 or rename Statistics fields, report columns, and the
gate comparison from p95/p99 to observed maximum; in
tmp/handoff-msq-prototype-done.md lines 12-14, restate the outcome as “max of 5
samples <= 500 ms” if retaining the current sample count.
- Around line 296-340: Update the three String.format templates in the dataset,
scenario, and EXPLAIN report loops to end with the literal newline escape \n
instead of the platform-dependent %n. Keep the existing formatting and report
content unchanged so all generated sections consistently use LF line endings.
- Around line 143-159: Update the report-writing flow in the scenario loop so
markdown(datasets, results, explains) is written once after the completed
scenario pair or at the end of the spaceCount loop, rather than before and after
each inner measurement. Preserve the initial incremental evidence behavior while
removing the per-iteration full-document rewrite.
- Around line 205-207: Update the COLD-phase flow around discardPlans() and
AuthorizedMultiSnapshotQuery.query so DISCARD PLANS executes on the same JDBC
connection used for the measured query, or explicitly qualify the report’s COLD
claim to acknowledge plans may not be discarded for that session. Preserve the
existing WARM behavior and concurrent benchmark structure.
- Around line 408-438: Update ExplainHighlight.from and its number helper to
parse the EXPLAIN JSON with the existing Jackson JSON tree support, then read
the root object at [0].Plan for Node Type and the root-level Planning Time,
Execution Time, Shared Hit Blocks, and Shared Read Blocks values. Remove the
regex-based Matcher.find extraction and preserve the current fallback values
when fields are absent; the fixed literal-field Pattern.quote logic is not the
issue.
- Around line 245-248: Update the timeout check in the exception handler around
the visible SQLException condition to rely solely on SQLState "57014". Remove
the getMessage() access and locale-dependent text matching so null messages and
localized PostgreSQL errors are handled as statement timeouts without changing
the existing traversal or return behavior.
- Around line 99-104: Strengthen the shadow-equivalence test around the compound
and per-space query results by asserting both result sets are non-empty and
contain the expected candidate count derived from the fixture setup. Keep the
existing validateRows and recursive equivalence assertions, anchoring the
changes in the test method containing compound.rows() and perSpace.rows().
- Around line 34-52: Update the static postgres field in
AuthorizedMultiSnapshotQueryIntegrationTests to declare PostgreSQLContainer with
its generic parameter, preferably PostgreSQLContainer<?> (or the appropriate
self-type), while preserving the existing container initialization and fluent
builder typing.
In `@tmp/handoff-msq-prototype-done.md`:
- Line 47: Update the handoff note to mark JetBrains IDE inspection as an
outstanding gate, not a completed validation, while retaining the Gradle compile
and test results. Explicitly state that inspection must be run only on the
edited backend Java files before merge.
- Around line 1-4: Remove the handoff record from tmp/ and place it under the
existing multi-snapshot query increment directory at
docs/increments/active/2026-08-05-multi-snapshot-query-prototype/, or exclude
the tmp/ copy from version control; ensure no committed durable artifact remains
under tmp/.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 55adecdc-d509-463e-9653-4ff448f0b69e
⛔ Files ignored due to path filters (1)
docs/increments/active/2026-08-05-multi-snapshot-query-prototype/results.mdis excluded by!docs/**
📒 Files selected for processing (9)
evaluation/README.mdevaluation/fixtures/retrieval-recall-golden-v1.jsonevaluation/pyproject.tomlevaluation/src/orgmemory_eval/retrieval_recall.pyevaluation/tests/test_retrieval_recall.pyintegrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQuery.javaintegrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQueryIntegrationTests.javaintegrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/MultiSnapshotSyntheticDataset.javatmp/handoff-msq-prototype-done.md
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: PostgreSQL GraphRAG
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Treat the repository and runtime evidence as the engineering system of record; do not treat chat or Northstar as authoritative.
Before changing a domain, read its specification, test-coverage document, and applicable decision filenames.
Material decisions about domain boundaries, authorization, persistence, publication, concurrency, cache isolation, parity scope, or deployment require an independent architecture challenge and documented alternatives before implementation.
Do not use completed increment documents as the source for current behavior; use them only for history or archaeology.
Before using unfamiliar Spring Boot, Spring Modulith, Spring AI, Gradle, React, Vite, Tailwind, TypeScript, Next.js, or Fumadocs APIs, consult current official documentation, Context7, and the relevant verification skill.
Readdocs/guidelines/agent-safety.mdbefore retrieval, AI, MCP, permission, upload, graph, or export work; never commit secrets or customer data.
Keepddl-auto=validateand pair every persisted-model change with a Flyway migration.
Use the testing harness; a terminating clean test is the JVM context gate, andbootRunis not verification.
Files:
evaluation/pyproject.tomlevaluation/README.mdevaluation/fixtures/retrieval-recall-golden-v1.jsontmp/handoff-msq-prototype-done.mdintegrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQuery.javaintegrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/MultiSnapshotSyntheticDataset.javaintegrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQueryIntegrationTests.javaevaluation/src/orgmemory_eval/retrieval_recall.pyevaluation/tests/test_retrieval_recall.py
**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Apply IDE inspection only to edited backend Java files.
Files:
integrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQuery.javaintegrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/MultiSnapshotSyntheticDataset.javaintegrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQueryIntegrationTests.java
🪛 ast-grep (0.45.0)
integrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQueryIntegrationTests.java
[warning] 433-434: Regular expression is compiled from a non-literal, possibly user-controlled value. A crafted regex (or input matched against one) can trigger catastrophic backtracking and hang the thread (ReDoS). Use a hardcoded literal pattern, wrap untrusted text with Pattern.quote(...), or validate/length-limit the input and enforce a matching timeout before passing it to Pattern.compile / String.matches / String.replaceAll / String.replaceFirst.
Context: Pattern.compile(String.format(
Locale.ROOT, NUMBER_FIELD.pattern(), Pattern.quote(field)))
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-java)
evaluation/src/orgmemory_eval/retrieval_recall.py
[info] 158-158: use jsonify instead of json.dumps for JSON output
Context: json.dumps(report, ensure_ascii=False, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 OpenGrep (1.26.0)
integrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQuery.java
[ERROR] 332-332: SQL query built via string concatenation passed to Statement.execute*(). Use PreparedStatement with parameterized queries instead.
(coderabbit.sql-injection.java-statement-concat)
🔇 Additional comments (18)
evaluation/fixtures/retrieval-recall-golden-v1.json (1)
1-112: LGTM!evaluation/pyproject.toml (1)
19-19: LGTM!evaluation/README.md (1)
29-50: LGTM!integrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/MultiSnapshotSyntheticDataset.java (8)
17-33: LGTM!
35-140: LGTM!
142-172: LGTM!
174-290: LGTM!
292-317: LGTM!
323-348: LGTM!
350-387: LGTM!
389-440: LGTM!integrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQuery.java (5)
31-54: LGTM!
195-224: LGTM!
244-289: LGTM!
421-588: LGTM!
124-143: 🚀 Performance & ScalabilityNo change needed for the
vector.embeddingcast.The baseline migration already creates a matching HNSW index for the expression
((embedding)::public.vector(1536))withvector_cosine_ops, so the query side matches that indexed expression.integrations/graph-rag-postgres/src/test/java/com/orgmemory/graphrag/postgres/AuthorizedMultiSnapshotQueryIntegrationTests.java (2)
58-82: LGTM!
255-272: LGTM!
fix(graph-rag): correct multi-snapshot gate evidence per PR #296 review
1 participant
Summary
ADR 0020 Phase 2 gate evidence (plan steps 1–3 of
2026-08-05-multi-snapshot-query-prototype). Implementation by Codexgpt-5.6-sol(high) under handoff; coordinator-reviewed with an amendment.AuthorizedMultiSnapshotQueryreturns identical candidates/attribution to N per-space queries across 1/7/20 spaces, narrow/broad grants. Test-scope only (Testcontainers); zero runtime wiring.ORDER BY … LIMIT). The store-fanned LATERAL variant the debate actually specified remains unmeasured and is the next evidence step.evaluation/; scoring needs live observations (plan step 3 continuation).ORGMEMORY_RUN_MSQ_BENCHMARK=true), excluded fromclean test.Per ADR 0020, cutover stays blocked on this evidence; Phase 1 remains the standing architecture.
Verification
./gradlew --no-daemon clean test: PASS 9m05s. Focused PostgreSQL shadow test: PASS.uv run --frozen pytest: 37 passed;ruff check: PASS.skip-release: test-scope prototype and gate evidence only; no product behavior changes.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests