refactor(knowledge): verify graph evidence through retrieval - #263
Conversation
…ph-evidence-verifier # Conflicts: # docs/specs/domains/secure-graph-rag.md # docs/specs/domains/secure-retrieval.md # docs/tests/domains/secure-graph-rag.md # docs/tests/domains/secure-retrieval.md
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughRetrieval now exposes ChangesGraph evidence verification boundary
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant GraphService
participant GraphEvidenceVerifier
participant VerifiedGraphEvidenceScope
participant SecureKnowledgeRetrievalStore
GraphService->>GraphEvidenceVerifier: verifyScope(actor, expectedAuthorizationModelId)
GraphEvidenceVerifier-->>GraphService: VerifiedGraphEvidenceScope
GraphService->>VerifiedGraphEvidenceScope: check authorization and scope consistency
GraphService->>GraphEvidenceVerifier: isCurrentGoverningEvidence(scope, spaceId, evidence)
GraphEvidenceVerifier->>SecureKnowledgeRetrievalStore: recheck canonical evidence
SecureKnowledgeRetrievalStore-->>GraphEvidenceVerifier: canonical candidates
GraphEvidenceVerifier-->>GraphService: current evidence result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.java (1)
196-202: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftFail-closed gap:
deactivate()does not verify the knowledge space is in the verified scope.
deactivate()only comparesresolved.authorizationGeneration(knowledgeSpaceId)against the caller-suppliedauthorizationGeneration.authorizationGeneration(UUID)returns a default of0LwhenknowledgeSpaceIdis absent from the verified scope. If the actor's scope no longer includes this knowledge space and the caller passesauthorizationGeneration = 0, the check0 != 0is false, so the guard silently passes andcurations.deactivate(...)proceeds against a space the actor is not verified to access.
apply()'srequireCurrentScope(Lines 249-251) guards against exactly this case with!resolved.includesKnowledgeSpace(spaceId) || resolved.authorizationGeneration(spaceId) != command.authorizationGeneration().deactivate()is missing the equivalentincludesKnowledgeSpacecheck. Add it here to fail closed consistently.As per path instructions, "Authorization must fail closed."
🔒 Proposed fix to restore fail-closed behavior in `deactivate()`
VerifiedGraphEvidenceScope resolved = resolve(actor, decision.policyVersion()); - if (resolved.authorizationGeneration(knowledgeSpaceId) - != authorizationGeneration) { + if (!resolved.includesKnowledgeSpace(knowledgeSpaceId) + || resolved.authorizationGeneration(knowledgeSpaceId) + != authorizationGeneration) { throw new KnowledgeRetrievalUnavailableException( "Knowledge graph authorization changed before curation"); }🤖 Prompt for 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. In `@core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.java` around lines 196 - 202, Update the authorization guard in deactivate() to fail when resolved does not include knowledgeSpaceId, in addition to checking the authorization generation mismatch. Preserve the existing exception and deactivation flow, matching apply()'s requireCurrentScope behavior so an absent scope cannot pass with generation 0.Source: Path instructions
core/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationServiceTests.java (1)
72-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new governing-evidence rejection branch and for
deactivate().
setUpSpaceAndEvidencestubsevidenceVerifier.isCurrentGoverningEvidenceto always returntrue. No test in this file stubs it to returnfalseand asserts thatrequireGoverningEvidencethrowsOrgMemoryAccessDeniedExceptionwith "Governing evidence is stale or unavailable". No test exercisesdeactivate()at all.Add both. The
deactivate()gap is important: it is the location of the fail-closed regression flagged inKnowledgeGraphCurationService.java(Lines 196-202). A regression test ondeactivate()that stubs a verified scope excluding the targetknowledgeSpaceIdwould catch that gap.
Do you want me to generate these test cases?🤖 Prompt for 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. In `@core/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationServiceTests.java` around lines 72 - 89, Add tests in KnowledgeGraphCurationServiceTests for the governing-evidence rejection path by overriding isCurrentGoverningEvidence to return false and asserting requireGoverningEvidence throws OrgMemoryAccessDeniedException with “Governing evidence is stale or unavailable”. Also add a deactivate() regression test using a verified scope that excludes the target knowledgeSpaceId, asserting the operation fails closed as expected.
🤖 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
`@core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.java`:
- Around line 291-301: Extract the duplicated try/catch authorization wrapper
from resolve() in
core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.java
lines 291-301 into a shared package-level helper or GraphEvidenceVerifier
default method, preserving the replacement message and exception propagation.
Update resolve() in
core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerService.java
lines 192-202 to delegate to the same helper, removing its duplicate wrapper.
In
`@core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportService.java`:
- Around line 120-126: Move the combined scope comparison from the duplicated
sameSpaceScope helpers in KnowledgeGraphExportService and
KnowledgeGraphExplorerService onto VerifiedGraphEvidenceScope as a
hasSameSpaceScope method accepting the other scope and knowledgeSpaceId. Update
both services to call this method and remove their private duplicate helpers,
preserving the authorizationModelId and hasSameAssetsAndGeneration checks.
In
`@core/src/main/java/com/orgmemory/core/knowledge/retrieval/VerifiedGraphEvidenceScope.java`:
- Around line 58-76: Make authorization fail closed for unknown Knowledge
Spaces: update VerifiedGraphEvidenceScope.forKnowledgeSpace and/or its accessor
contract to reject IDs absent from assetIdsByKnowledgeSpace, and in
KnowledgeGraphExportService after verifyScope call
resolved.includesKnowledgeSpace(knowledgeSpaceId) and throw accessDenied() when
false; preserve the existing authorized export flow.
- Around line 58-76: Make the accessors in VerifiedGraphEvidenceScope fail
closed for knowledge spaces not included in the scope: update forKnowledgeSpace
and authorizationGeneration to reject an unincluded space rather than returning
an empty asset set or generation 0. Verify callers perform
includesKnowledgeSpace checks first, preserving existing behavior for included
spaces and ensuring comparison helpers cannot treat absent spaces as equivalent.
- Around line 100-111: Update VerifiedGraphEvidenceScope.toRetrievalScope to
support a knowledgeSpaceId parameter and build the RetrievalScope using only
asset IDs from that space, while preserving the existing organization, actor,
authorization, and evaluation metadata. Update callers performing evidence
rechecks to use the per-space scope so verification cannot include assets from
other Knowledge Spaces.
In
`@core/src/test/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifierTests.java`:
- Around line 129-174: Add a sixth mismatched-candidate case in
rejectsMissingDuplicateOrMismatchedCanonicalCandidates that keeps
organizationId, chunkId, sourceRevisionId, and currentAclSnapshotId equal to the
exact candidate while varying only knowledgeAssetId, and retain the
corresponding assertFalse call so the recheck comparison covers all five
identity fields.
---
Outside diff comments:
In
`@core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.java`:
- Around line 196-202: Update the authorization guard in deactivate() to fail
when resolved does not include knowledgeSpaceId, in addition to checking the
authorization generation mismatch. Preserve the existing exception and
deactivation flow, matching apply()'s requireCurrentScope behavior so an absent
scope cannot pass with generation 0.
In
`@core/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationServiceTests.java`:
- Around line 72-89: Add tests in KnowledgeGraphCurationServiceTests for the
governing-evidence rejection path by overriding isCurrentGoverningEvidence to
return false and asserting requireGoverningEvidence throws
OrgMemoryAccessDeniedException with “Governing evidence is stale or
unavailable”. Also add a deactivate() regression test using a verified scope
that excludes the target knowledgeSpaceId, asserting the operation fails closed
as expected.
🪄 Autofix (Beta)
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: f38c4668-3a6d-4d63-a07f-e1bb103eb666
⛔ Files ignored due to path filters (5)
docs/increments/active/2026-07-31-spring-modulith-package-refactor/plan.mdis excluded by!docs/**docs/specs/domains/secure-graph-rag.mdis excluded by!docs/**docs/specs/domains/secure-retrieval.mdis excluded by!docs/**docs/tests/domains/secure-graph-rag.mdis excluded by!docs/**docs/tests/domains/secure-retrieval.mdis excluded by!docs/**
📒 Files selected for processing (15)
ARCHITECTURE.mdcore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerConfiguration.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerService.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportService.javacore/src/main/java/com/orgmemory/core/knowledge/graph/package-info.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifier.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/GraphEvidenceVerifier.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/VerifiedGraphEvidenceScope.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/package-info.javacore/src/test/java/com/orgmemory/core/ModulithVerificationTests.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationServiceTests.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerServiceTests.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportServiceTests.javacore/src/test/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifierTests.java
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Backend · Java 25
🧰 Additional context used
📓 Path-based instructions (3)
**/*
📄 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:
core/src/main/java/com/orgmemory/core/knowledge/retrieval/GraphEvidenceVerifier.javaARCHITECTURE.mdcore/src/main/java/com/orgmemory/core/knowledge/retrieval/package-info.javacore/src/main/java/com/orgmemory/core/knowledge/graph/package-info.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerConfiguration.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/VerifiedGraphEvidenceScope.javacore/src/test/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifierTests.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportServiceTests.javacore/src/test/java/com/orgmemory/core/ModulithVerificationTests.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerServiceTests.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifier.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerService.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationServiceTests.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportService.java
**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
Apply IDE inspection only to edited backend Java files.
Files:
core/src/main/java/com/orgmemory/core/knowledge/retrieval/GraphEvidenceVerifier.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/package-info.javacore/src/main/java/com/orgmemory/core/knowledge/graph/package-info.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerConfiguration.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/VerifiedGraphEvidenceScope.javacore/src/test/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifierTests.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportServiceTests.javacore/src/test/java/com/orgmemory/core/ModulithVerificationTests.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerServiceTests.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifier.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerService.javacore/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationServiceTests.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportService.java
core/src/main/java/com/orgmemory/core/{authorization,knowledge,permission}/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/com/orgmemory/core/{authorization,knowledge,permission}/**/*.java: Treat PostgreSQL ACL evidence as canonical and OpenFGA as the relationship
authorization decision point. Authorization must fail closed. Filtering
must happen before ranking, LIMIT, graph traversal, answer generation,
export, and citation rendering. Flag metadata or timing leak paths.
Files:
core/src/main/java/com/orgmemory/core/knowledge/retrieval/GraphEvidenceVerifier.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/package-info.javacore/src/main/java/com/orgmemory/core/knowledge/graph/package-info.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerConfiguration.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/VerifiedGraphEvidenceScope.javacore/src/main/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifier.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerService.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.javacore/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportService.java
🔇 Additional comments (15)
core/src/main/java/com/orgmemory/core/knowledge/retrieval/GraphEvidenceVerifier.java (1)
1-18: LGTM!core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationService.java (1)
3-7: LGTM!Also applies to: 46-46, 56-64, 81-83, 222-255, 257-269, 270-289, 303-314
core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerService.java (1)
4-6: LGTM!Also applies to: 43-61, 100-102, 116-116, 156-156, 409-412
core/src/main/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerConfiguration.java (1)
3-3: LGTM!Also applies to: 20-27
core/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphCurationServiceTests.java (1)
3-5: LGTM!Also applies to: 51-52, 64-64, 77-88
core/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExplorerServiceTests.java (1)
3-5: LGTM!Also applies to: 71-72, 89-89, 100-100, 213-215, 231-234
core/src/test/java/com/orgmemory/core/knowledge/graph/KnowledgeGraphExportServiceTests.java (1)
3-5: LGTM!Also applies to: 46-47, 57-57, 67-68, 103-105
core/src/main/java/com/orgmemory/core/knowledge/retrieval/VerifiedGraphEvidenceScope.java (1)
23-43: LGTM!core/src/main/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifier.java (1)
11-47: LGTM!Also applies to: 49-78
core/src/test/java/com/orgmemory/core/knowledge/retrieval/CanonicalGraphEvidenceVerifierTests.java (2)
55-127: LGTM!
176-200: LGTM!core/src/main/java/com/orgmemory/core/knowledge/retrieval/package-info.java (1)
9-14: LGTM!core/src/main/java/com/orgmemory/core/knowledge/graph/package-info.java (1)
4-7: LGTM!core/src/test/java/com/orgmemory/core/ModulithVerificationTests.java (1)
399-406: LGTM!Also applies to: 723-730
ARCHITECTURE.md (1)
167-172: LGTM!
|
Addressed the two outside-diff findings in ebd3bba: deactivate now requires explicit Space membership before generation comparison, and curation tests cover both stale governing evidence and absent-Space deactivation. Post-review verification: focused Graph/verifier plus full Modulith, full Core, docs, release policy, and terminating clean test (99 tasks) all pass. |
1 participant
Summary
Verification
skip-release: intermediate modular refactor; release follows completion of the full refactor goal