fix(delete): require explicit group scope for chunk/source episode deletes - #412
Conversation
…letes Conn::remove_episodes_by_chunk_id and remove_episodes_by_source took Option<&[&str]> group_ids and silently ran an unscoped, all-groups DETACH DELETE whenever the caller omitted the filter. The liminis app omits it on every call, so an ordinary heading rename could delete another group's episode data. Make group_ids mandatory (&[&str]) at the data-access layer so an unscoped query is unrepresentable, and add extract_required_group_ids in handlers.rs (factored out of handle_delete_by_group's existing validation) so both handle_delete_chunk_episode and handle_delete_by_source reject missing/null/empty group_ids with an actionable error instead of defaulting to "all groups". Update the MCP tool schemas to mark group_ids required for both tools.
|
🏭 Fabrik — stage: Implement Everything looks correct and matches the plan exactly. All work is committed, pushed, tests pass (1250 passed, 8 ignored), and clippy is clean. Now emitting the PR creation marker. Made Used 23/100 turns, 4.0M input (45 raw + 3.9M cache-read + 177k cache-write) / 5k output tokens. |
|
Warning Review limit reached
Next review available in: 59 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughDeletion by source and deletion by chunk now require explicit, non-empty Possibly related issues
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5571edfe-50a3-4884-a14a-602e75a0adbd
📒 Files selected for processing (7)
CHANGELOG.mdcrates/core/src/db.rscrates/core/src/handlers.rscrates/core/tests/tier1c_deletion.rscrates/service/src/mcp/tools.rscrates/service/tests/mcp_real_corpus_mutation_e2e.rsspecs/406-knowledge-delete-chunk-episode/spec.md
![handarbeit-pruefer[bot]](https://avatars.githubusercontent.com/in/4408765?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the changes closing #406: knowledge_delete_chunk_episode and knowledge_delete_by_source now require an explicit, non-empty group_ids, enforced at both the MCP schema level (required in tools.rs) and the data-access layer (Conn::remove_episodes_by_chunk_id/remove_episodes_by_source now take group_ids: &[&str] instead of Option), making an unscoped delete unrepresentable rather than merely blocked in the handler. The new extract_required_group_ids helper correctly rejects absent/null/empty arrays and non-string/empty-string elements, dedupes while preserving order, and is shared cleanly with handle_delete_by_group's existing validation. extract_optional_group_ids, used by read/search handlers where "absent = all groups" is intended, is untouched. Cypher queries are properly parameterized with group_id IN $gids, and the second-phase DETACH DELETE only touches UUIDs already scoped by the first query. Verified both lcg-core and lcg-service compile cleanly with these changes, checked all call sites of the two Conn methods (only the two handlers use them), and reviewed the new rejection/cross-group-isolation tests in tier1c_deletion.rs, which correctly assert that rows in other groups survive an unscoped or wrongly-scoped call. No correctness, security, or design issues found; the PR's own caveat about the liminis-app rollout sequencing (client not yet sending group_ids) is explicitly out of scope for this repo and accurately documented.
There was a problem hiding this comment.
Pull request overview
This PR closes #406 by removing the dangerous “omit group_ids = delete across all groups” behavior from knowledge_delete_chunk_episode and knowledge_delete_by_source, making cross-group destructive deletes unrepresentable and rejected at the handler boundary.
Changes:
- Made
Conn::remove_episodes_by_chunk_id/remove_episodes_by_sourcerequire an explicit group scope (noOption), and updated the handlers to enforce a non-emptygroup_ids. - Updated MCP tool schemas to mark
group_idsas required for both delete tools. - Expanded and updated deletion tests to cover rejection of unscoped calls and cross-group isolation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/406-knowledge-delete-chunk-episode/spec.md | Adds Spec Kit spec defining required group scoping and test scenarios for the fix. |
| crates/core/src/db.rs | Makes group scoping mandatory in the DB deletion queries. |
| crates/core/src/handlers.rs | Enforces required group_ids for destructive deletes via a shared helper. |
| crates/core/tests/tier1c_deletion.rs | Updates existing tests and adds new cross-group isolation + rejection tests. |
| crates/service/src/mcp/tools.rs | Updates MCP tool schemas to require group_ids for the two delete tools. |
| crates/service/tests/mcp_real_corpus_mutation_e2e.rs | Updates E2E mutation test to pass group_ids to chunk deletion. |
| CHANGELOG.md | Documents the patch behavior change under 0.13.2. |
Suppressed comments (2)
crates/service/src/mcp/tools.rs:376
- Same as above: the schema marks
group_idsas required, but it still permits an empty array. AddingminItems: 1(and non-empty string items) makes the schema accurately reflect the handler contract and avoids a round-trip just to discover the request is invalid.
"group_ids": {
"type": "array", "items": {"type": "string"},
"description": "Non-empty array of groups to restrict deletion to (required). An unscoped, all-groups delete is not supported."
}
crates/core/src/db.rs:909
group_idsis documented as mandatory and non-empty, but an empty slice is still representable at the type level. A defensiveis_empty()check here prevents accidental unscoped behavior or DB-specific edge cases aroundIN [], and makes failures clearer if a new call site ever regresses.
pub fn remove_episodes_by_chunk_id(
&self,
chunk_id: &str,
group_ids: &[&str],
) -> Result<Vec<String>, Error> {
let match_sql =
"MATCH (ep:Episodic) WHERE ep.name = $name AND ep.group_id IN $gids RETURN ep.uuid";
let params = serde_json::json!({ "name": chunk_id, "gids": group_ids });
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🏭 Fabrik — stage: Review All required review checks pass locally (fmt, full test suite of 1250 tests, clippy with Review FindingsVerified
Fixed
Blocking
Reviewed the mandatory-group-scope fix for Used 28/100 turns, 1.6M input (37 raw + 1.5M cache-read + 71k cache-write) / 6k output tokens. |
…layer CodeRabbit and Copilot both flagged that the required `group_ids` MCP schemas still permitted an empty array, and that the DB-layer functions documented group_ids as mandatory/non-empty without enforcing it. Add `minItems: 1` to both delete tool schemas for earlier client-side validation, and a defensive `is_empty()` check in `remove_episodes_by_source`/`remove_episodes_by_chunk_id` that returns `Error::Ipc` rather than silently running an empty `IN $gids` clause.
|
🏭 Fabrik — stage: Review (review feedback addressed) All three threads resolved. No issue body changes are needed — this was a code-only fix in response to review bot feedback, not a spec change. SummaryAddressed three related review-bot findings (CodeRabbit, GitHub Copilot) on PR #412:
Verified: Threads addressed:
Resolved 3 review thread(s) across 6 comment(s). |
There was a problem hiding this comment.
Reviewed the diff for PR #412 (via gh pr diff since the local checkout lacked main/a remote). The change closes the unscoped-delete gap cleanly: group_ids is now mandatory end-to-end — enforced in the JSON schema (minItems: 1), in the shared extract_required_group_ids handler helper, and defensively in the DB layer (remove_episodes_by_source/remove_episodes_by_chunk_id now take &[&str] and reject empty slices). Both prior review threads (CodeRabbit's minItems request and Copilot's DB-layer defense-in-depth request) are fully addressed with no new defects introduced by the fix. I verified the two DB functions have no other callers left on the old Option signature, and that extract_optional_group_ids is still correctly scoped to the five read/search handlers. Test coverage (rejection for omitted/null/empty, and cross-group isolation for both name and source-prefix matching) looks thorough. Only issue found is a stale doc comment left over from the refactor.
Additional findings (could not anchor to diff)
crates/core/src/handlers.rs:1325: This doc comment (and the inline comment just below at the let group_ids line) still describes the pre-refactor state — group_ids validation for handle_delete_by_group used to be inline here, contrasted explicitly with extract_optional_group_ids. Now that validation is delegated to the shared extract_required_group_ids helper (used by all three destructive handlers), the comment's framing ("validated explicitly here", "silently dropping a malformed element") no longer matches what the code at this call site actually does — the real explanation now lives on extract_required_group_ids itself. Worth a follow-up touch-up so a future reader isn't misled about where the validation logic lives.
|
🏭 Fabrik — stage: Validate All requirements verified against the spec, the full local gate (fmt/test/clippy) passes clean, and the branch is rebased and mergeable. Here is the validation report. Validation ReportRequirements: 7/7 FRs and 5/5 SCs passed
Test Suite: PASSED
Regressions: None detected
Code completeness
Branch state
Verdict: READY TO MERGEValidation passed. PR mergeable: MERGEABLE, mergeStateStatus: BLOCKED (CI in progress on HEAD, not a conflict). All 7 FRs / 5 SCs verified against spec, full local suite passes (1250 tests, fmt/clippy clean), no regressions, branch rebased and pushed. Used 29/100 turns, 1.6M input (41 raw + 1.6M cache-read + 71k cache-write) / 5k output tokens. |
…equired_group_ids refactor Comment described the old inline validation logic that was moved into the shared extract_required_group_ids helper; it no longer matched what this call site does.
|
🏭 Fabrik — stage: Validate (review feedback addressed) Fix applied, verified, committed, and pushed. This was a bot-review finding (stale doc comment), not an explicit user "resolved" signal, so I'm not advancing the pipeline. Fixed the stale doc comment on Threads addressed: Resolved 0 review thread(s) across 1 comment(s). |
There was a problem hiding this comment.
Reviewed the fix that makes group_ids mandatory for knowledge_delete_chunk_episode and knowledge_delete_by_source, closing the unscoped-delete gap in issue #406. The change is layered correctly: the new extract_required_group_ids handler helper rejects absent/null/empty/malformed group_ids with an actionable error before reaching the DB layer; Conn::remove_episodes_by_chunk_id/remove_episodes_by_source now take &[&str] instead of Option<...> and additionally defend against an empty slice at the data-access layer; and the MCP tool schemas mark group_ids required with minItems: 1, addressing both prior CodeRabbit/Copilot review threads. Single-group calls route WAL flush to that group's writer directly, multi-group calls fall back to the documented default-group routing (consistent with handle_delete_by_group's existing behavior). Test coverage is thorough — rejection tests for omitted/null/empty group_ids that verify no cross-group data loss, and cross-group isolation tests for both exact and prefix source matches. No correctness issues found; note (already called out in the PR description) that the liminis app's own call sites don't send group_ids yet, which is a deployment-sequencing concern tracked separately (liminis#998) rather than a defect in this PR.
|
🏭 Fabrik — stage: Validate Confirmed all three call sites ( Validation ReportRequirements: 7/7 FRs and 5/5 SCs passed
Test Suite: PASSED
Regressions: None detected
Code completeness
Branch state
Verdict: READY TO MERGEValidation passed. PR #412 mergeable: MERGEABLE, mergeStateStatus: BLOCKED (pending CI, not a conflict). All 7 FRs / 5 SCs verified against spec, full local suite passes (1250 tests, 0 failed, fmt/clippy clean), no regressions since last validation — only change was a doc-comment fix. Used 18/100 turns, 1.2M input (33 raw + 1.2M cache-read + 60k cache-write) / 4k output tokens. |
|
🏭 Fabrik — stage: Validate (review feedback addressed) The No action is required — this is an informational bot finding with no requested fix, not an explicit user "resolved" signal, so I'm not making any changes and not signaling stage completion. SummaryReviewed the Threads addressed: Resolved 0 review thread(s) across 1 comment(s). |
… wording CHANGELOG's 0.13.2 section read as a pure bug fix; add a ### Changed subsection marking the group_ids requirement as BREAKING for any caller that previously omitted it. Also correct the spec's description of the liminis app's two unscoped delete calls: they are reached via an if/else on isChunkEntry(entry), not invoked back to back. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the group-scoping fix for knowledge_delete_chunk_episode/knowledge_delete_by_source across db.rs, handlers.rs, tools.rs, and the test suites. The implementation matches the PR description closely: Conn::remove_episodes_by_source/remove_episodes_by_chunk_id now take a mandatory &[&str] with a defense-in-depth empty check, extract_required_group_ids correctly rejects absent/null/empty/malformed values while deduping, both MCP schemas now require group_ids with minItems: 1 (resolving the two prior review threads), and single-vs-multi-group WAL routing correctly falls back to the documented default-group limitation only for genuinely multi-group calls. Tests cover rejection (omitted/null/[]) with cross-group survival checks and scoped-deletion isolation for both source and chunk-id matching, including exact and prefix cases. No other internal call sites of the changed Conn methods exist outside the delete handlers, and the WAL replay path doesn't re-invoke these methods, so the signature change is safe. No defects found.
|
🏭 Fabrik — stage: Validate Everything checks out. All requirements from the spec are met, the requested CHANGELOG breaking-change callout and spec wording fix are both in place, the local gate (fmt/test/clippy) is clean, and the branch is rebased with no conflicts. Writing the final validation report. Validation ReportRequirements: 7/7 FRs and 5/5 SCs passed
User's Validate-stage feedback: addressed
Test Suite: PASSED
Regressions: None detectedBranch state
Verdict: READY TO MERGEValidation passed. PR #412 mergeable: MERGEABLE, mergeStateStatus: BLOCKED (pending CI, not a conflict). All 7 FRs / 5 SCs verified, the requested CHANGELOG breaking-change callout and spec wording fix are confirmed present, full local suite passes (1250 tests, fmt/clippy clean). Used 13/100 turns, 907k input (23 raw + 846k cache-read + 61k cache-write) / 3k output tokens. |
|
🏭 Fabrik merge-train — Landed via batch PR #415. |
Closes #406
Summary
knowledge_delete_chunk_episodeandknowledge_delete_by_sourcesilently deletedEpisodicrows across every group whenever the caller omittedgroup_ids(or passednull/[]). The liminis app omits it on every call, so an ordinary heading rename — which enqueues onedeleteChunkEpisode/deleteBySourceper affected chunk — could destroy another group's episode data. This is the same failure class as #368, which 0.13.0 treated as release-blocking.Both methods now require an explicit, non-empty
group_idsand reject the call outright (naming the missing parameter) rather than defaulting to "all groups."Changes
crates/core/src/db.rs:Conn::remove_episodes_by_chunk_idandremove_episodes_by_sourcetakegroup_ids: &[&str](no longerOption), so an unscoped query is unrepresentable at the data-access layer, not merely blocked one layer up.crates/core/src/handlers.rs: addedextract_required_group_ids(non-empty array of non-empty strings, deduped, actionable error) and switchedhandle_delete_chunk_episode/handle_delete_by_sourceto it.handle_delete_by_group's existing inline validation was refactored to share the same helper.extract_optional_group_idsis untouched — still correctly used by the five read/search handlers where "absent = all groups" is the intended semantic.crates/service/src/mcp/tools.rs:group_idsmoved intorequiredfor both tool schemas; descriptions updated to state the scope is mandatory.crates/core/tests/tier1c_deletion.rs): fixed the 5 existing tests to pass explicitgroup_ids, and added rejection tests (omitted/null/[], asserting both groups' rows survive) and cross-group isolation tests (samenameorsource_description/prefix colliding across two groups, scoped delete only touches the named group) for both methods.crates/service/tests/mcp_real_corpus_mutation_e2e.rs: added the now-requiredgroup_idsto its one unscopedknowledge_delete_chunk_episodecall.CHANGELOG.md: added a[0.13.2]entry.Notes for reviewers
group_idsstill routes throughDEFAULT_GROUP_ID's writer, per the existing Investigate: knowledge_delete_chunk_episode attributes a cross-group delete to a single WAL stream #402-deferred rationale. This issue narrows Investigate: knowledge_delete_chunk_episode attributes a cross-group delete to a single WAL stream #402 but doesn't resolve it, by design.indexing-queue.ts,knowledge-writer-provider.ts) don't sendgroup_idsyet — that fix is tracked asliminis#998.liminis-app/scripts/build-liminis-context-graph.shbuilds this server from an unpinned sibling checkout, so the next app build after this merges would bundle a server that rejects the app's own unscoped calls untilliminis#998also lands. This is a deployment-sequencing concern for whoever cuts the next liminis app release, not something this PR can fix.Test plan
cargo fmt --all— cleancargo test— 1250 passed, 8 ignored (64 suites)cargo clippy --all-targets -- -D warnings— no issues