feat: split oversized chunk_text and add chunk_id resubmission idempotency - #286
feat: split oversized chunk_text and add chunk_id resubmission idempotency#286verveguy wants to merge 35 commits into
Conversation
|
🏭 Fabrik — stage: Implement Everything is clean, pushed, and the diff is tightly scoped to this issue. Now let's emit the PR creation marker. Implemented issue #284 in full: Used 91/100 turns, 0k input / 28k output tokens. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds configurable character-based splitting for oversized Assessment against linked issues
Possibly related issues
Possibly related PRs
Comment |
![handarbeit-pruefer[bot]](https://avatars.githubusercontent.com/in/4408765?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have what I need to write the review.
I reviewed the core of this PR: crates/core/src/chunk_split.rs, the handle_knowledge_process_chunk idempotency/split logic in crates/core/src/handlers.rs, the supporting Db queries in db.rs, the telemetry event, and the ADR/tests that document the design's known trade-offs. (No main ref was available locally to diff against, so I read the changed files directly against the PR description.) The splitter and reconstruction logic are well-designed and thoroughly tested for the sequential/single-caller case, and the ADR is candid about several trade-offs. I found two issues worth flagging: an unhandled max_chars == 0 case in the splitter that hangs the async runtime rather than degrading gracefully as documented, and a TOCTOU gap where the same-chunk_id idempotency check is not atomic with the eventual insert, so concurrent resubmissions of the same chunk_id can still produce duplicate episodes despite the feature's stated goal.
|
🏭 Fabrik — stage: Review Review FindingsFixed
Verified
Noted, not fixed (low-severity, pre-existing design trade-off)
The PR is clean, fully tested, and ready for human review. Reviewed the #284 implementation (chunk splitting + chunk_id resubmission idempotency). Found and fixed one real bug: Used 50/100 turns, 10k input / 22k output tokens. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
🏭 Fabrik — stage: Review (review feedback addressed) No uncommitted changes — nothing new to push since finding [1] was already fixed in the prior Review pass. Response to review commentsFinding [1] — Finding [2] — I did not implement a fix in this pass. The only correct general fix is a new per- Given the failure mode is self-healing (not silent data loss — the next resubmission of that No issue body changes are needed — this doesn't change the spec's scope, only records review-triage decisions. Threads addressed:
Resolved 2 review thread(s) across 2 comment(s). |
There was a problem hiding this comment.
I have everything needed for the review now.
Summary
I reviewed the diff against main (merge-base 5c388d5), focusing on crates/core/src/chunk_split.rs and the idempotency/split logic in crates/core/src/handlers.rs, plus the supporting db.rs, telemetry.rs, test, and doc changes. The splitter itself is solid — the length and lossless-concatenation invariants are well tested, and the previously-fixed max_chars == 0 infinite loop is correctly clamped. However, the idempotency mechanism this PR's summary centers on has two correctness gaps worth fixing before merge: a TOCTOU race that lets concurrent resubmissions for the same chunk_id create duplicate episodes despite the advertised idempotency guarantee, and a delimiter-collision bug where a chunk_id/source_file that happens to contain a #i/N-shaped substring (a plausible real-world convention for "part i of N" chunk naming) gets misparsed as a stale split marker, causing every resubmission to be treated as Anomalous and needlessly replaced instead of no-op'd. A smaller issue: a no-op resubmission silently ignores a changed source_file, echoing the new value in the response while the persisted episode metadata still reflects the old one.
|
🏭 Fabrik — stage: Validate Local and remote are in sync, working tree clean. All validation checks pass. Validation ReportRequirements: 9/9 FRs verified
Edge cases verified
Test Suite: PASSED
Regressions: None detected
Branch state
Verdict: READY TO MERGE (pending CI completion)Validation passed: all 9 FRs verified against the spec, full local test suite green (805 passed, 0 failed), clippy/fmt clean, no regressions. PR #286 mergeable: MERGEABLE, mergeStateStatus: BLOCKED (CI Used 24/100 turns, 15k input / 5k output tokens. |
b58a72e to
bb36699
Compare
There was a problem hiding this comment.
Review Summary
I reviewed the chunk-splitting and idempotency implementation (crates/core/src/chunk_split.rs, the handle_knowledge_process_chunk rewrite and idempotency-reconstruction helpers in crates/core/src/handlers.rs, the supporting Db::get_episodes_by_chunk_id/remove_episodes_by_chunk_id queries, the updated tests in ipc_parity.rs/tier1c_deletion.rs, and the ADR). (Note: this sandbox's git history is a single detached, parentless commit with no main ref and no remote configured, so I could not run git diff main...HEAD as instructed — this review is based on reading the current file contents in full instead.)
The splitter itself (chunk_split.rs) is careful and well-tested: char-count-based (not byte-based), whitespace-preferring with a hard-cut fallback, provably lossless via the concat() == text tests, and the max_chars == 0 clamp is a legitimate fix for a real hang.
The bigger concern is in handle_knowledge_process_chunk's concurrency model. The idempotency lookup-and-possible-delete runs atomically under state.write_lock, but that guard is dropped (handlers.rs:578) before the actual add_episode insert(s) happen; add_episode only reacquires write_lock briefly for its own commit phase, after doing (possibly slow, LLM-bound) extraction unlocked. This means two concurrent knowledge_process_chunk calls for the same chunk_id (a very plausible scenario for a PR whose entire point is safe client retries) can both observe PriorState::None/no conflict and both proceed to insert, producing duplicate episodes for one chunk_id — the opposite of the guarantee this PR sets out to add. The ADR explicitly discusses interleaving for a different chunk_id mid-split as benign, but doesn't address same-chunk_id concurrent resubmission, which isn't covered by that reasoning.
I also found a narrower correctness edge case: parse_unit_suffix derives unit markers by scanning the entire source_description (which embeds the caller-supplied chunk_id/source_file verbatim) for a trailing #i/N. A chunk_id or source_file that happens to end in something matching that shape (e.g. chunk_id = "notes#2/5") will be misparsed as a split-unit marker on a never-split episode, pushing reconstruction into Anomalous and silently defeating idempotency (safe — it just always replaces instead of no-op'ing — but surprising and defeats the entire feature for such chunk_ids).
Everything else (response shape additivity, telemetry event, WAL flush reuse, test renames matching the new semantics) looks consistent with the stated design.
reconstruct_prior_chunk_text's single-row branch previously probed parse_unit_suffix on the lone row before treating it as the never-split shape. Since parse_unit_suffix scans the whole source_description for the last '#' with no anchor to where the caller-controlled portion ends, a chunk_id or source_file that happens to look like "page#3/7" was misparsed as a stale split-unit marker, forcing every resubmission of that chunk_id into the replace path (re-running extraction) even on a byte-identical retry. A single row is now unconditionally treated as the never-split shape. A genuine single-row leftover from a partial split failure still self-heals correctly: its content is only one unit's fragment, so comparing it against the caller's full chunk_text still falls through to the replace path. Found by review during Validate (PR #286).
There was a problem hiding this comment.
I reviewed the diff for PR #286 (chunk splitting + chunk_id resubmission idempotency), focusing on crates/core/src/chunk_split.rs, the handle_knowledge_process_chunk handler and its new PriorState/ChunkResubmission reconstruction logic in crates/core/src/handlers.rs, the updated tests in ipc_parity.rs and tier1c_deletion.rs, and the accompanying ADR/telemetry/README docs.
The splitter (chunk_split.rs) is correct and well-tested: it guarantees the lossless/order-preserving invariant the rest of the design leans on, handles multi-byte chars via chars() throughout, and clamps max_chars to avoid a zero-width infinite loop. The reconstruct_prior_chunk_text/parse_unit_suffix logic, including the just-added fix for chunk_ids that look like a split suffix (e.g. "page#3/7"), is sound, and the response-shape and test changes described in the PR match what's actually in the diff.
The one substantive issue is a concurrency gap: the lookup-decide-(maybe delete) step acquires write_lock and releases it immediately, before the fresh ingest happens, and each subsequent add_episode call reacquires the lock only briefly and independently. Two concurrent knowledge_process_chunk calls for the same chunk_id (a realistic scenario given this PR is explicitly about making retries safe) can both pass the lookup step with the same stale view, and both then ingest independently — producing duplicate/divergent episodes under one chunk_id, exactly the pre-PR behavior the idempotency feature is meant to eliminate, or worse, a lost update when the texts differ. The ADR's "Consequences" section only discusses interleaving across different chunk_ids or across units within one call, not concurrent calls for the same chunk_id, and there's no test for it.
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: 2e330251-8c81-498c-8f26-86f231b46a0b
📒 Files selected for processing (13)
README.mdcrates/core/src/chunk_split.rscrates/core/src/db.rscrates/core/src/handlers.rscrates/core/src/lib.rscrates/core/src/telemetry.rscrates/core/tests/ipc_parity.rscrates/core/tests/tier1c_deletion.rscrates/service/src/mcp/tools.rsdocs/adr/0052-chunk-splitting-and-chunk-id-idempotency.mddocs/adr/index.mddocs/telemetry.mdspecs/284-knowledge-process-chunk-internal/spec.md
The Consequences section covered interleaving between different chunk_ids and between split units, but not the narrower same-chunk_id race: write_lock is dropped after the lookup/reconstruct/conditional- delete step and only briefly reacquired per unit at add_episode's Phase C commit, so two concurrent calls for the same chunk_id can both observe the same prior state and both insert. This has been flagged repeatedly in review as a known, accepted limitation pending a follow-up issue for per-(group_id, chunk_id) request-duration locking; this documents it explicitly rather than leaving it undiscoverable from the ADR alone, per review feedback on PR #286. Also fixes a minor wording nit (overused "exactly").
|
🏭 Fabrik — stage: Validate (review feedback addressed) No conflicts, still mergeable. No issue-body changes are needed — this is procedural review triage, not a scope change. SummaryProcessed four new review comments on PR #286:
Docs-only change (no Rust touched), so no rebuild/retest was needed; Given the TOCTOU race remains a genuine, unfixed data-integrity gap (now documented rather than hidden, but not resolved), I'm not signaling completion here — this stays open for a maintainer decision on whether it blocks merge or ships as a documented, tracked limitation. Threads addressed:
Resolved 4 review thread(s) across 4 comment(s). |
There was a problem hiding this comment.
Confirmed verbatim storage. I have enough to write the review now.
I reviewed the working tree at the PR's head commit (675b120). Note: the sandbox's git checkout is shallow/detached with no main ref and no remote configured, so git diff main...HEAD wasn't possible; I instead read the relevant files directly (chunk_split.rs, handlers.rs's handle_knowledge_process_chunk/reconstruct_prior_chunk_text/parse_unit_suffix, db.rs's new get_episodes_by_chunk_id, episode.rs, telemetry, docs, ADR-0052, and the updated tests) and traced the logic against the PR description's claims.
Overall the implementation is careful and matches its own design doc closely. The splitter is verified lossless and char-boundary-safe (tested against multibyte, unbreakable-token, and zero-max_chars edge cases), the source_description suffix scheme correctly uses rfind/rsplit_once so a chunk_id that itself contains #i/N-shaped text can't be misparsed as a stale unit marker (there's even a regression test for exactly this), and body/content is stored verbatim so the idempotency reconstruction's concatenation is provably exact. The one substantive design concern — that the write lock is released between the lookup/delete step and the actual insert, so two concurrent resubmissions of the same chunk_id can both observe the same prior state and both insert, producing duplicate episodes — is already called out explicitly as a known, accepted limitation in ADR-0052's Consequences section with a proposed follow-up (per-(group_id, chunk_id) locking). I'm flagging it below for visibility/discussion rather than as an unnoticed bug, since it does undercut the idempotency guarantee in exactly the retry-during-slow-extraction scenario this feature targets.
|
🏭 Fabrik — stage: Validate All checks pass. Writing the final validation report. Validation ReportRequirements: 9/9 FRs verified
Test Suite: PASSED
Regressions: None detectedDiff is scoped to Known, documented limitation (not blocking)Same- Branch state
Verdict: READY TO MERGEValidation passed: all 9 FRs verified against the spec, full local test suite green (816 passed, 0 failed), clippy/fmt clean, no regressions. PR #286 mergeable: MERGEABLE, mergeStateStatus: BLOCKED (CI Used 23/100 turns, 18k input / 9k output tokens. |
|
🏭 Fabrik — stage: Validate (review feedback addressed) Addressed the review comment by adding an explicit caveat to the README and the SummaryThe review comment flagged that the same- Fix applied: added a short caveat sentence to both:
Both now state that concurrent calls for the same Verified: Not signaling stage completion — this was a documentation fix in response to review feedback, not an explicit user signal that the issue is fully resolved. Threads addressed:
Resolved 1 review thread(s) across 1 comment(s). |
|
🏭 Fabrik — stage: Validate All consistent. Everything checks out: full local gate is clean, requirements are met, branch is rebased and pushed, and the PR is mergeable (not dirty). Time to write the validation report. Validation passed. PR mergeable: MERGEABLE, mergeStateStatus: BLOCKED (CI Validation ReportRequirements: 9/9 verified
Test Suite: PASSED
Regressions: None detectedDiff scoped to Known, documented limitation (not blocking)Same- Branch state
Verdict: READY TO MERGEUsed 16/100 turns, 21k input / 3k output tokens. |
|
🏭 Fabrik — stage: Validate (review feedback addressed) Good, still mergeable. No issue body changes are needed — this was a targeted code/doc fix in response to review feedback, not a scope change. SummaryFixed both review findings on PR #286:
Verified: Not signaling stage completion — this was review-feedback triage, not an explicit signal that the issue is fully resolved. Threads addressed:
Resolved 2 review thread(s) across 2 comment(s). |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/handlers.rs (1)
36-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalize a zero threshold before using it.
0remains the handler’s threshold whilesplit_into_unitssilently uses1. Thus every non-empty request is “oversized,” telemetry advertises a threshold of0, and large input triggers one extraction per character.Proposed fix
- .and_then(|v| v.parse().ok()) + .and_then(|v| v.parse::<usize>().ok()) + .map(|limit| limit.max(1)) .unwrap_or(8_000)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 87799333-d156-4b49-ad7f-a432d93c3d0a
📒 Files selected for processing (5)
README.mdcrates/core/src/chunk_split.rscrates/core/src/handlers.rscrates/service/src/mcp/tools.rsdocs/telemetry.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/telemetry.md
- README.md
- crates/service/src/mcp/tools.rs
|
🏭 Fabrik — stage: Validate CI's All checks pass. Emitting the validation report and stage completion. Validation ReportRequirements: 9/9 verified
Test Suite: PASSED
Regressions: None detectedDiff scoped to Known, documented limitation (not blocking)Same- Branch state
Verdict: READY TO MERGEValidation passed. PR mergeable: MERGEABLE, mergeStateStatus: BLOCKED (CI Used 19/100 turns, 23k input / 4k output tokens. |
There was a problem hiding this comment.
Now I have everything needed to write the review.
Review
I reviewed the diff against main (gh pr diff 286), focusing on crates/core/src/handlers.rs (the resubmission/split logic), the new chunk_split.rs splitter, db.rs's new lookup query, and the updated tests/docs. The splitter itself is well-tested (whitespace preference, hard-cut fallback, multibyte safety, lossless concatenation, zero-max_chars guard) and the idempotency reconstruction logic correctly guards against the "chunk_id shaped like a split suffix" false-positive case with a dedicated regression test. The main concerns are a cross-endpoint data-safety issue introduced by the new lookup-by-name step, and the lock granularity chosen for that lookup.
|
🏭 Fabrik — stage: Validate (review feedback addressed) No issue body changes are needed — this was targeted code/doc fixes in response to review feedback, not a scope change. SummaryFixed all four review findings on PR #286:
Verified: Not signaling stage completion — this was review-feedback triage, not an explicit signal that the issue is fully resolved. Threads addressed:
Resolved 4 review thread(s) across 4 comment(s). |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/core/tests/ipc_parity.rs (1)
1103-1106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese assertions silently depend on
LCG_CHUNK_TEXT_ADVISORY_MAX_CHARSbeing unset.The threshold is resolved once into a
OnceLock, so any environment that exports this variable (a dev shell, a CI job pinning a smaller value) makes the hardcoded8000/div_ceil(8000)expectations fail with a confusing diff rather than a clear skip. Reading the same helper the handler uses, or asserting the value from the responsewarning.recommended_max_chars, would keep the tests self-consistent.Also applies to: 1245-1246
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4585908b-c644-46bf-bfe0-d7e277eb247a
📒 Files selected for processing (5)
crates/core/src/db.rscrates/core/src/handlers.rscrates/core/src/telemetry.rscrates/core/tests/ipc_parity.rsdocs/adr/0052-chunk-splitting-and-chunk-id-idempotency.md
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/core/src/telemetry.rs
There was a problem hiding this comment.
Summary
I reviewed the knowledge_process_chunk split/idempotency implementation (crates/core/src/chunk_split.rs, crates/core/src/handlers.rs), the DB helpers it relies on (crates/core/src/db.rs), the telemetry event, and the new/renamed tests in crates/core/tests/ipc_parity.rs and tier1c_deletion.rs. (Note: this checkout only contains a single squashed commit with no main ref to diff against, so I reviewed the code directly rather than via git diff main...HEAD.)
The splitter (chunk_split.rs) is solid — the lossless/threshold invariants are well tested, including multibyte and unbreakable-token edge cases. The reconstruction logic in handlers.rs (reconstruct_prior_chunk_text) carefully guards against foreign name collisions from knowledge_add_episode, which is good defensive design and well tested.
Two real gaps stand out, both around the core idempotency claim the PR is built on ("a chunk_id never silently accumulates unrelated episodes across calls"):
- Changing
source_fileon a resubmission of the samechunk_idsilently breaks lineage tracking — the old episode(s) become permanently untracked/orphaned while a new episode is created, so the chunk_id does end up accumulating episodes across calls, contradicting the documented guarantee. This isn't mentioned in the README, ADR, or tests. - The check-then-act sequence (read-lock lookup, then a separate/no lock for the actual insert or write-lock for delete) is not atomic, so two concurrent resubmissions of a brand-new
chunk_idcan both observe "no prior state" and both insert, producing duplicate episodes. This one is at least explicitly disclosed in the README as a known limitation, so I'm flagging it for confirmation rather than as an oversight.
There was a problem hiding this comment.
This is comprehensive and correct. I have enough to write the final review now.
Review Summary
I reviewed the full diff against main (13 files, ~1900 insertions): the new chunk_split.rs splitter, the idempotency/reconstruction logic added to handlers.rs (reconstruct_prior_chunk_text, ChunkResubmission, the reworked handle_knowledge_process_chunk), the new Db::get_episodes_by_chunk_id/remove_episodes_by_uuids methods, telemetry, docs, and the associated test suite (14 new/modified knowledge_process_chunk tests plus splitter unit tests).
The design is careful and internally consistent: splitting is verified lossless (unit concatenation reproduces the source exactly), the source_description encoding keeps Episodic.name untouched so existing chunk_id-based deletion is unaffected, and the idempotency reconstruction logic correctly guards against name collisions with knowledge_add_episode rows, malformed/partial prior state, and both directions of threshold-crossing replacement. Replace-before-delete ordering avoids leaving a chunk_id with zero episodes on a failed re-ingest. The known TOCTOU race on concurrent same-chunk_id submissions is explicitly documented (tracked as #288) rather than silently present, and the residual knowledge_add_episode-lineage-collision gap is also explicitly accepted in the ADR. Test coverage is thorough, including the previously-untested reverse threshold-crossing case, unbreakable-token hard-cuts, and foreign name-collision handling. I did not find correctness bugs, security issues, or data-loss scenarios in this diff; the few notes below are minor/informational.
| // the fresh ingest runs. Two callers racing on the same chunk_id — including two first-time | ||
| // submissions — can both observe the same prior_state and both proceed to insert, producing | ||
| // duplicate/divergent episodes until a later resubmission self-heals via the Anomalous path. | ||
| let db_for_lookup = load_db(&state)?; |
There was a problem hiding this comment.
The read-lock lookup and the subsequent ingest/delete run without holding a single lock across the whole operation (documented TOCTOU limitation, tracked in #288). This is a deliberate, documented tradeoff rather than an oversight, but worth flagging in review since it means concurrent resubmissions (including two first-time submissions) of the same chunk_id can still produce duplicate/divergent episodes until a later serialized resubmission self-heals via the Anomalous path. If callers rely on this endpoint for exactly-once semantics under concurrency, that expectation should be enforced (e.g. per-chunk_id serialization) rather than left to caller discipline.
| // `name` happens to equal this `chunk_id`). Leave it untouched: not counted, not | ||
| // deleted. | ||
| // | ||
| // Residual gap, explicitly accepted as a top-level decision (see ADR-0284's Consequences |
There was a problem hiding this comment.
Accepted gap: a knowledge_add_episode row whose caller-chosen name equals a chunk_id and whose source_description happens to also end in :{chunk_id} (or a valid #i/N suffix) will be silently pulled into this chunk_id's lineage and can be deleted on a later replace. This is explicitly called out in the ADR as accepted risk, but since source_description is fully caller-controlled on knowledge_add_episode, this is a plausible (if unlikely) foreign-data-deletion path rather than a purely theoretical one.
There was a problem hiding this comment.
I've reviewed the core implementation in crates/core/src/handlers.rs and crates/core/src/chunk_split.rs, along with the test changes in ipc_parity.rs and tier1c_deletion.rs, and the doc updates (README, telemetry.md).
Summary
I reviewed the knowledge_process_chunk splitting/idempotency implementation (crates/core/src/handlers.rs, crates/core/src/chunk_split.rs), the reworked test suite (ipc_parity.rs, tier1c_deletion.rs), and the accompanying docs (README, telemetry.md, ADR-0284).
The design is careful and the code shows evidence of prior review passes (the commit history shows earlier automated-review fixes already applied): the source_description encoding/decoding is symmetric and defends against a chunk_id that itself contains # or looks like a split suffix; the splitter is lossless and its edge cases (unbreakable tokens, multibyte chars, zero-max_chars) are unit-tested; the replace-after-successful-ingest ordering avoids leaving a chunk_id with zero episodes on a failed re-ingest; and remove_episodes_by_uuids is only ever called with UUIDs derived from reconstruct_prior_chunk_text's own lineage reconstruction, so it can't be tricked into deleting a foreign episode. The behavior-change tests (idempotent no-op, threshold-crossing replace in both directions, foreign-name collision) match the implementation.
The one substantive gap is the documented TOCTOU race: the read lock used for the prior-state lookup is dropped before the fresh ingest runs, so two concurrent calls for the same chunk_id (including two first-time submissions) can both observe PriorState::None and both insert, producing duplicate episodes under the same chunk_id until a later serialized resubmission self-heals via the Anomalous path. This is called out in code comments, the ADR, and the README, and tracked in issue #288 — but it does mean the "resubmission idempotency" guarantee this PR advertises doesn't actually hold under concurrent load, which is worth flagging explicitly since a caller relying on FR-006 without reading the caveat could be surprised.
| let chunk_id_lookup = chunk_id.clone(); | ||
| let group_id_lookup = group_id.clone(); | ||
| let chunk_text_for_compare = chunk_text.clone(); | ||
| let read_guard = state.write_lock.read().await; |
There was a problem hiding this comment.
The read lock covering the prior-state lookup is released (line 684) before the fresh ingest and any write-lock-protected delete happens. Two concurrent knowledge_process_chunk calls for the same chunk_id — including two first-time submissions — can both observe PriorState::None and both proceed to insert, producing duplicate episodes under one chunk_id that violate the idempotency guarantee this PR advertises (FR-006/FR-007) until a later non-concurrent resubmission happens to self-heal it via the Anomalous path. This is documented in the surrounding comment, the ADR, and the README, and tracked as #288, but it's a real gap in the headline guarantee for any caller that fires resubmissions concurrently (e.g. a naive retry-on-timeout without a request lock), so it's worth calling out prominently rather than leaving it as an accepted known limitation.
There was a problem hiding this comment.
I've reviewed enough to write the final assessment. This is a very high quality, thoroughly documented and tested PR. I found no significant bugs — only a couple of minor observations worth flagging.
Review Summary
I reviewed the full diff scope of PR #286: the new chunk_split.rs splitter, the reworked handle_knowledge_process_chunk idempotency/resubmission logic in handlers.rs, the DB helper functions (get_episodes_by_chunk_id, remove_episodes_by_uuids), telemetry, and the updated test suite (ipc_parity.rs, tier1c_deletion.rs), plus the README/telemetry docs and ADR-0284.
The implementation is careful and well-reasoned: the splitter is verifiably lossless and byte-safe on multi-byte UTF-8 boundaries, the source_description parsing correctly disambiguates a caller-chosen chunk_id that itself looks like a split suffix (page#3/7, tested explicitly), the replace-path deletion is deferred until after a successful re-ingest (so a failed replace never leaves zero episodes for a chunk_id), and both threshold-crossing directions (single→split, split→single) are covered by dedicated tests. The known TOCTOU race between the read-lock lookup and the write-lock ingest is explicitly documented in code comments and tracked in a separate issue (#288), which is a reasonable scope boundary rather than an oversight.
I found no high or critical issues. A couple of low/medium observations below are worth a look but don't affect correctness of the delivered scope.
| episode_uuids: uuids, | ||
| } | ||
| } | ||
| PriorState::Reconstructed { uuids, .. } | PriorState::Anomalous { uuids } => { |
There was a problem hiding this comment.
PriorState::Anomalous rows are unconditionally routed to Replace and their prior UUIDs deleted after a successful re-ingest. This is the documented self-healing behavior for a mid-split partial failure, but it also applies to the residual-gap case noted in the reconstruct_prior_chunk_text comment (a foreign knowledge_add_episode row whose source_description happens to end in ":{chunk_id}"): that foreign row will be silently deleted on the next knowledge_process_chunk resubmission for the same chunk_id, not just reconstructed into. The ADR calls this out as an accepted top-level decision, so this is a documentation note rather than a defect, but worth confirming reviewers are aware the exposure is deletion, not just misattribution.
| /// run past the `max_chars` mark, cuts near `start` and repeats — producing a run of small units | ||
| /// until the whitespace-free stretch is exhausted. Invariants (unit length, lossless | ||
| /// concatenation) still hold; this is a quality/performance caveat for pathological input, not a | ||
| /// correctness issue. If that pattern repeats across the whole input, each unit's scan still |
There was a problem hiding this comment.
The documented pathological worst case (O(chars.len() * max_chars) when whitespace only appears near the start of each max_chars window) is mitigated by running the split in spawn_blocking, but there's no upper bound on max_chars itself — LCG_CHUNK_TEXT_ADVISORY_MAX_CHARS is parsed from env with no validation/clamping (chunk_text_advisory_max_chars() in handlers.rs just falls back to 8000 on a parse failure, but accepts any successfully-parsed usize::MAX-ish value). A misconfigured very large threshold combined with pathological input could still produce a long-blocking task on the shared blocking-thread pool. Minor given this is operator-controlled configuration, not attacker input.
There was a problem hiding this comment.
Everything matches the PR description accurately. This is a well-scoped, thoroughly-documented, and self-critical change. I'm confident in the review now.
Note: I could not git diff main...HEAD as instructed — this checkout is a single shallow commit with no main ref or remote available locally, so I reviewed the full current state of the relevant files (chunk_split.rs, handlers.rs, db.rs, tests, docs) against the PR description instead.
Review
I reviewed the chunk-splitting logic (crates/core/src/chunk_split.rs), the idempotency-reconstruction and dispatch logic in handle_knowledge_process_chunk (crates/core/src/handlers.rs), the supporting Db query/delete methods, the changed tests in ipc_parity.rs and tier1c_deletion.rs, and the README/telemetry/ADR documentation. (I was unable to run git diff against main — this checkout has no base ref or remote available — so I read the current state of the files directly and cross-checked it against the PR description's claims.)
The implementation is careful and unusually well-documented: the splitter's whitespace-preference/hard-cut-fallback logic is provably lossless and panic-safe (all cut points land on char boundaries, and the loop always makes forward progress), the source_description encode/decode convention is symmetric and guards against a chunk_id that itself looks like a split suffix (e.g. "page#3/7"), and the foreign-row / anomalous-state handling in reconstruct_prior_chunk_text is conservative in the right direction (never deletes something it isn't sure belongs to the lineage). The delete-after-successful-ingest ordering correctly avoids leaving a chunk_id with zero episodes on a failed replace. Tests exercise the described behavior changes and match what the code actually does.
The one substantive issue is a genuine TOCTOU race: the read lock guarding the prior-state lookup is dropped before the fresh ingest runs, so two concurrent submissions for the same chunk_id (including two first-time submissions) can both proceed and insert, producing duplicate/divergent episodes. This is explicitly acknowledged in the code comments, README, and ADR, and tracked as a follow-up (#288), but it's still a real behavior gap worth flagging since it affects correctness under concurrent load, which is the kind of thing that's easy to hit in a multi-agent ingestion pipeline.
| // a resubmission that no-ops (the common retry case) never blocks concurrent ingestion of | ||
| // unrelated chunk_ids behind an exclusive lock. | ||
| // | ||
| // KNOWN LIMITATION (see ADR-0284, tracked in #288): this read guard is dropped below, before |
There was a problem hiding this comment.
The read lock is dropped before the fresh ingest/insert runs, so two concurrent knowledge_process_chunk calls for the same chunk_id (e.g. a client retry racing the original in-flight request, or two first-time submissions) can both observe PriorState::None/NoOp and both proceed to insert, producing duplicate or divergent episodes for one chunk_id. This is explicitly documented here and in the README/ADR as a known limitation tracked by #288, but it's a real correctness gap under concurrent load that isn't otherwise guarded against (e.g. no per-chunk_id mutex or unique constraint).
There was a problem hiding this comment.
Good, edge cases are covered well. This confirms high test quality. I'll finalize my review now.
Summary
I reviewed the knowledge_process_chunk splitting and idempotency implementation for PR #286: crates/core/src/chunk_split.rs (the whitespace-preferred splitter), the reconstruction/idempotency logic and dispatch flow in crates/core/src/handlers.rs, the supporting db.rs query/delete methods, episode.rs's content-storage path, the telemetry event, and the associated tests/docs/ADR.
The implementation is careful and well-reasoned throughout: the splitter's lossless-concatenation and per-unit-length invariants are directly unit-tested including multibyte and unbreakable-token edge cases; the source_description parsing that reconstructs prior chunk_text correctly handles ambiguous chunk_ids containing #/: characters (verified with regression tests); deletion is correctly scoped to only the UUIDs derived from lineage reconstruction, never a caller-supplied list; and add_episode stores content verbatim, preserving the reconstruction invariant end-to-end. The mid-split partial-failure self-heal path (Anomalous state) is well-designed in principle but has no direct test exercising it — the existing tests cover the parsing edge cases and the clean replace/no-op paths, but not a simulated partial-ingest failure feeding back into a subsequent resubmission. The PR text itself explicitly discloses two accepted design trade-offs (a TOCTOU race between the read-lookup and the eventual delete, tracked as #288; and a residual name-collision ambiguity with knowledge_add_episode-created rows) — both are real correctness caveats worth surfacing in review even though the authors already made and documented a deliberate call on them.
No new defects were found beyond what the PR authors already disclosed and reasoned through.
| // a resubmission that no-ops (the common retry case) never blocks concurrent ingestion of | ||
| // unrelated chunk_ids behind an exclusive lock. | ||
| // | ||
| // KNOWN LIMITATION (see ADR-0284, tracked in #288): this read guard is dropped below, before |
There was a problem hiding this comment.
Documented and tracked as #288, but worth flagging in review: the read lock covering the chunk_id lookup is dropped before the fresh ingest/delete runs, so two concurrent calls for the same chunk_id (including two first-time submissions) can both observe PriorState::None and both insert, producing duplicate episodes until a later non-concurrent resubmission self-heals via the Anomalous path. This is an accepted, tracked limitation rather than an oversight, but it does mean the idempotency guarantee this PR advertises does not hold under concurrent load on the same chunk_id.
| // `name` happens to equal this `chunk_id`). Leave it untouched: not counted, not | ||
| // deleted. | ||
| // | ||
| // Residual gap, explicitly accepted as a top-level decision (see ADR-0284's Consequences |
There was a problem hiding this comment.
Accepted-but-real gap: a knowledge_add_episode row whose caller-chosen name equals a chunk_id and whose caller-chosen source_description happens to end in ":{chunk_id}" gets silently absorbed into this chunk_id's lineage on a later knowledge_process_chunk call — it can be deleted by a replace it has nothing to do with. The ADR documents this as an accepted top-level decision, but there's no guard rail (e.g. requiring the source_description prefix to originate from this handler) to prevent an unrelated write from being swept into a replace.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_knowledge_process_chunk_splits_oversized_text() { |
There was a problem hiding this comment.
No test exercises the PriorState::Anomalous self-heal path for a genuinely mid-split partial failure (e.g. unit 3 of 5 fails after units 1-2 already committed, then the same chunk_id is resubmitted). The parsing-edge-case tests (duplicate_chunk_id_looks_like_split_suffix, ignores_foreign_name_collision) and the clean replace/no-op tests are thorough, but the specific scenario called out in the PR description ("An unparseable or partial prior state... is treated as a mismatch and replaced — which is also how a failed split self-heals on retry") has no direct regression coverage.
There was a problem hiding this comment.
Now I have everything needed to write the review.
The PR implements bounded splitting for oversized chunk_text and chunk_id resubmission idempotency in knowledge_process_chunk. I reviewed crates/core/src/chunk_split.rs, the core logic in crates/core/src/handlers.rs (reconstruct_prior_chunk_text, handle_knowledge_process_chunk), the db.rs query/delete helpers, and the accompanying tests in ipc_parity.rs and tier1c_deletion.rs. The implementation is unusually well documented — invariants, edge cases, and known limitations are called out inline and cross-referenced to the ADR and to tracking issue #288. The splitter is provably lossless and char-boundary-safe, the lineage-reconstruction parser correctly disambiguates split-unit suffixes from caller-supplied chunk_ids that merely look like one (verified against the dedicated regression test), and the delete-after-successful-ingest ordering avoids leaving a chunk_id with zero episodes on partial failure. Test coverage for the new logic (split, no-op, replace, threshold-crossing in both directions, unbreakable-token hard cut, foreign name collision) is thorough and matches the implementation. The one substantive issue is the acknowledged TOCTOU window between the read-lock lookup and the write-lock delete/ingest — real, but already tracked in #288, so I've flagged it for visibility rather than as new information.
| let chunk_id_lookup = chunk_id.clone(); | ||
| let group_id_lookup = group_id.clone(); | ||
| let chunk_text_for_compare = chunk_text.clone(); | ||
| let read_guard = state.write_lock.read().await; |
There was a problem hiding this comment.
The prior-state lookup runs under a read lock that is dropped (line 684) before the fresh ingest/delete proceeds. Two concurrent resubmissions of the same chunk_id (including two first-time submissions) can both observe PriorState::None (or the same Reconstructed state) and both proceed to insert, producing duplicate or divergent episodes for one chunk_id until a later call self-heals via the Anomalous path. This is called out in the comment and tracked in #288, so it's a known/accepted gap rather than new — flagging for visibility since it is a real correctness issue under concurrent load on the same chunk_id.
There was a problem hiding this comment.
Documentation matches implementation accurately. I have enough to write the review now.
I reviewed the core implementation of this PR: crates/core/src/chunk_split.rs (the splitter), crates/core/src/handlers.rs (handle_knowledge_process_chunk, reconstruct_prior_chunk_text, PriorState/ChunkResubmission), crates/core/src/db.rs (get_episodes_by_chunk_id, remove_episodes_by_uuids, remove_episodes_by_chunk_id), the telemetry event, the MCP tool description, the ADR, README, and the new/changed tests in ipc_parity.rs and tier1c_deletion.rs. (Note: the working tree is a shallow single-commit checkout with no main ref available, so I reviewed the post-change file contents directly rather than a diff against base.)
The implementation is unusually careful: the splitter is lossless and char-safe (operates on byte offsets, never collects a Vec<char>), the idempotency reconstruction correctly disambiguates this handler's own source_description lineage from foreign name collisions (with a dedicated regression test for a chunk_id shaped like a split suffix, e.g. "page#3/7"), deletion ordering avoids leaving a chunk_id with zero episodes on a failed replace, and the known TOCTOU race between the read-lock lookup and the eventual write is explicitly documented and cross-referenced to a tracking issue (#288) rather than silently left. Tests are thorough and align with the documented behavior changes (ipc_parity.rs, tier1c_deletion.rs). I found one substantive, already-acknowledged correctness gap worth flagging in-line for visibility, and a couple of minor points; no security issues or data-loss bugs beyond what's already called out in code comments.
| let chunk_id_lookup = chunk_id.clone(); | ||
| let group_id_lookup = group_id.clone(); | ||
| let chunk_text_for_compare = chunk_text.clone(); | ||
| let read_guard = state.write_lock.read().await; |
There was a problem hiding this comment.
The read guard on state.write_lock is dropped (line 684) before the fresh ingest/replace runs. Two concurrent knowledge_process_chunk calls for the same chunk_id — including two first-time submissions — can both observe PriorState::None/the same prior text and both proceed to insert, producing duplicate or divergent episodes for one chunk_id until a later resubmission self-heals via the Anomalous path. This is explicitly documented here and tracked as #288, so it's a known, deferred limitation rather than an oversight — but it does mean the idempotency guarantee this PR delivers only holds for serialized callers, which is easy to miss when consuming the tool's advertised behavior.
| // deleted. | ||
| // | ||
| // Residual gap, explicitly accepted as a top-level decision (see ADR-0284's Consequences | ||
| // section): a `knowledge_add_episode` row whose `source_description` *also* happens to |
There was a problem hiding this comment.
Acknowledged residual gap (documented in the comment and ADR-0284's Consequences section): if a knowledge_add_episode caller sets source_description to end with :{chunk_id} (mirroring this handler's own convention) or with a parseable #i/N suffix, that foreign row gets pulled into knowledge_process_chunk's idempotency lineage for that chunk_id — it can be silently deleted by a later replace. This is a narrower cross-endpoint data-integrity edge case than the general foreign-name-collision case (which is correctly handled and tested), and it's called out as an accepted trade-off rather than fixed, so flagging for visibility rather than as a hard defect.
| @@ -0,0 +1,406 @@ | |||
| # ADR-0284: Chunk-Splitting and Chunk-ID Idempotency for `knowledge_process_chunk` | |||
There was a problem hiding this comment.
Minor: the ADR/PR narrative states #282 becomes redundant and should be closed once this merges, but that's a repo-process action outside this PR's diff — worth double-checking it actually gets done post-merge since nothing in this PR enforces it.
There was a problem hiding this comment.
I have enough to write the final review now.
Reviewed the full diff of crates/core/src/chunk_split.rs, crates/core/src/handlers.rs's handle_knowledge_process_chunk/reconstruct_prior_chunk_text, the telemetry event definition, and the associated tests in ipc_parity.rs and tier1c_deletion.rs. Note: this environment's git checkout is a single-commit shallow clone with no main ref available, so I reviewed the current file contents directly against the PR description rather than a literal git diff main...HEAD.
The splitting algorithm is correct and well-tested (lossless concatenation, char-boundary-safe, whitespace preference, hard-cut fallback, max_chars == 0 degenerate case). The idempotency reconstruction logic in handlers.rs is thorough and its edge cases (foreign name collisions, mixed split/non-split shapes, incomplete/duplicate unit sequences) are handled conservatively via the Anomalous → replace path. The one substantive issue is a documented-but-real TOCTOU race: the read lock used for the prior-state lookup is dropped before the fresh ingest/delete runs, so two concurrent resubmissions of the same chunk_id (including two first-time submissions) can both observe PriorState::None and both ingest, producing duplicate episodes until a later call self-heals via the Anomalous path. The PR author already tracks this as issue #288 and calls it out explicitly in code comments and the ADR, but it's worth flagging here since it is a real correctness gap in this PR's own idempotency guarantee under concurrent load, not just a theoretical one.
| let chunk_id_lookup = chunk_id.clone(); | ||
| let group_id_lookup = group_id.clone(); | ||
| let chunk_text_for_compare = chunk_text.clone(); | ||
| let read_guard = state.write_lock.read().await; |
There was a problem hiding this comment.
The read lock guarding the prior-state lookup is released (drop(read_guard) at line 684) before the fresh ingest/delete runs. Two callers racing on the same chunk_id — including two first-time submissions — can both observe PriorState::None (or the same Reconstructed/Anomalous state) and both proceed to ingest, producing duplicate or divergent episodes for one chunk_id until a later resubmission happens to self-heal via the Anomalous path. This directly undercuts the idempotency guarantee (FR-006/FR-007) under concurrent resubmission, which is a realistic scenario for a retry-on-timeout client. Already tracked as issue #288 and documented in the surrounding comment/ADR, but flagging since it's a genuine gap in this PR's stated correctness property, not just a stylistic caveat.
There was a problem hiding this comment.
Reviewed the current state of the PR's head commit (cae7602, "Address remaining automated-review findings on PR #286"). This repo checkout is a shallow single-commit clone with no main ref available locally, so I could not run git diff main...HEAD; instead I inspected the commit message's three stated changes directly against the current files: the unit_count→expected_n rename in reconstruct_prior_chunk_text (handlers.rs), the added #288 cross-reference in the TOCTOU-race code comment, and the new test_knowledge_process_chunk_replace_crosses_threshold_reverse test. All three match their stated intent and look correct — the rename eliminates the shadowing without changing behavior, the comment correctly cross-links the tracked concurrency issue at the code site, and the new test exercises a real, previously-untested gap (a prior split lineage being fully replaced by a single below-threshold episode). No new defects found in this incremental commit. The extensive TOCTOU/concurrency discussion from earlier review passes remains an accepted, documented, and tracked (#288) limitation, consistent with the code comments and prior maintainer replies — not reopened here.
There was a problem hiding this comment.
Reviewed the final commit on PR #286 ("Address remaining automated-review findings"), which addresses three items from the prior review round: renaming a locally-shadowed unit_count to expected_n in reconstruct_prior_chunk_text, cross-linking issue #288 directly in the TOCTOU-race code comment, and adding test_knowledge_process_chunk_replace_crosses_threshold_reverse to cover replacing a split lineage with a single below-threshold episode. (Note: this checkout has no main ref or remote configured, so I reviewed the current state of the affected files — chunk_split.rs, handlers.rs, ipc_parity.rs — directly rather than via git diff main...HEAD.)
All three changes are correct: the rename eliminates the shadowing without affecting the unit_count response/telemetry field it was confused with, the #288 reference is now inline at the TOCTOU comment site, and the new reverse-direction test correctly asserts the split lineage's UUIDs are fully captured in replaced_uuids and the response collapses to the singular episode_uuid shape. This PR's substantive design (splitting, idempotency reconstruction, the TOCTOU concurrency gap) was already covered at length across many prior review threads, all resolved with reasoned replies pointing to ADR-0284/#288 as the accepted tradeoff. No new defects found in this commit.
There was a problem hiding this comment.
Reviewed the full diff for PR #286: crates/core/src/chunk_split.rs (new splitter), crates/core/src/handlers.rs (handle_knowledge_process_chunk, reconstruct_prior_chunk_text, ChunkResubmission/PriorState), the associated test changes in crates/core/tests/ipc_parity.rs and tier1c_deletion.rs, and the docs/ADR updates. This diff (cae7602) is the "address remaining automated-review findings" follow-up commit; I could not diff directly against main in this checkout (single flattened commit, no remote/history), so I read the final state of every touched file against the PR description and the extensive list of already-resolved review threads.
The splitter is correctly lossless and bounded (verified the backward-whitespace-scan termination logic and the hard-cut fallback), the lock-lifetime/TOCTOU concurrency gap is the same accepted, tracked (#288) limitation already discussed at length across prior threads, the source_description encoder/decoder convention now has the cross-referencing comments that were requested, unit_count's telemetry semantics match the corrected docs, and the new reverse-direction (split→single-episode replace) test looks sound. I did not find any new defect introduced by this commit's changes (the expected_n rename, the #288 comment cross-link, or the new test) beyond what's already tracked in the resolved threads above.
There was a problem hiding this comment.
Reviewed the full PR #286 diff (chunk_split.rs, handlers.rs, db.rs, telemetry.rs, tools.rs, ADR-0284, and the test suite changes in ipc_parity.rs/tier1c_deletion.rs) against the base main. This is the mature, feedback-addressed state of the branch: the splitter's lossless-concatenation and always-terminating-scan properties check out, reconstruct_prior_chunk_text's source_description parsing correctly distinguishes never-split/split/foreign-collision cases and falls back safely to Anomalous on any inconsistency, the replace-path ordering (ingest-then-delete) avoids ever leaving a chunk_id with zero episodes on a failed re-ingest, and Episodic.content is stored verbatim by episode::add_episode so the concatenation-based idempotency comparison is exact. The extensive prior review threads (TOCTOU concurrency gap tracked as #288, mid-split partial-failure orphaning, the unit_count/expected_n shadowing fix, the # delimiter cross-reference comment, DoS-shaped splitter cost) are all still accurately reflected in the current code and none of the fixes introduce new regressions. Test coverage for the resubmission matrix (no-op, replace, threshold-crossing in both directions, source_file rename, foreign name collision, chunk_id shaped like a split suffix) is thorough and each test's assertions match the documented behavior. No new defects found beyond what's already tracked or accepted in this PR's threads.
There was a problem hiding this comment.
Reviewed the full knowledge_process_chunk/knowledge_delete_chunk_episode idempotency and splitting implementation (crates/core/src/chunk_split.rs, crates/core/src/handlers.rs), the reconstruction/no-op/replace logic, the new and renamed tests in ipc_parity.rs and tier1c_deletion.rs, and the docs/ADR updates. Note: this checkout has a single squashed commit with no reachable main ref, so a literal git diff main...HEAD wasn't possible; I read the current state of every file the PR description calls out directly instead.
The implementation is internally consistent: the splitter's lossless-concatenation invariant holds under manual trace, the source_description encoder (handlers.rs, split-ingest loop) and decoder (reconstruct_prior_chunk_text) use matching delimiter conventions, the threshold boundary (chars_count > threshold vs. the splitter's <= max_chars single-unit fast path) is consistent at the edge, and the newly added reverse-direction test (split → below-threshold replace) correctly asserts every prior unit gets replaced. The TOCTOU concurrency gap, mid-split partial-failure orphaning, telemetry unit_count semantics, and the foreign-source_description-collision edge case are all pre-existing, already-discussed limitations that the current diff (renaming unit_count→expected_n, cross-linking #288, adding the reverse-threshold test) doesn't newly introduce or regress. I found no new defects to report.
There was a problem hiding this comment.
Reviewed crates/core/src/handlers.rs (the knowledge_process_chunk rewrite: PriorState/ChunkResubmission reconstruction, the split-ingest loop, replace-path deletion), crates/core/src/chunk_split.rs (the whitespace-preferred splitter and its unit tests), the telemetry ChunkTextOversized event, and the test changes in ipc_parity.rs/tier1c_deletion.rs including the newly added test_knowledge_process_chunk_replace_crosses_threshold_reverse and the expected_n rename/#288 cross-reference from this PR's latest commit. (Note: the checkout available to this review has no main ref/history to diff against — a single squashed commit — so this was a full read of the relevant files against the PR description rather than a line-level diff.)
The splitter is lossless and terminates correctly (verified the backward whitespace-scan bound and the zero-max_chars clamp by hand); the idempotency reconstruction correctly restricts lineage membership by chunk_id suffix, handles mixed/inconsistent prior shapes as Anomalous, and the DB-layer functions (get_episodes_by_chunk_id, remove_episodes_by_uuids) scope/trust boundaries as documented. The extensive set of prior review threads on this PR already covers the substantive design tradeoffs here in depth (the TOCTOU race across the lookup/ingest/delete sequence, mid-split partial-failure orphaning, the source_description provenance-collision edge case, and the splitter's pathological-input cost) and each has either a tracked follow-up (#288) or a reasoned accepted-tradeoff response from the author. I did not find any new defect introduced by the latest commit (the unit_count→expected_n rename, the #288 code comment, or the new reverse-threshold-crossing test) or elsewhere in this diff that isn't already tracked by those threads.
There was a problem hiding this comment.
Reviewed the chunk-splitting and chunk_id-idempotency implementation (crates/core/src/chunk_split.rs, crates/core/src/handlers.rs's handle_knowledge_process_chunk/reconstruct_prior_chunk_text, and the associated tests/docs). Note: this checkout has no main ref available locally (single detached commit, no remote), so I reviewed the code at HEAD directly rather than a git diff main...HEAD; the PR's extensive existing review-thread history covers the substantive design tradeoffs (TOCTOU races on concurrent resubmission, mid-split partial-failure orphaning, splitter worst-case cost, source_description delimiter conventions) in detail, all marked resolved/acknowledged with reasoned replies from the author.
The head commit's own changes — renaming a locally-scoped unit_count to expected_n to stop shadowing the response-level field, cross-linking issue #288 directly in the TOCTOU code comment, and adding a new test covering the reverse (split → below-threshold single-episode) replace direction — are each correct and consistent with the rest of the implementation. The splitter's invariants (bounded unit size, lossless concatenation, char-boundary safety, termination) and the reconstruction/idempotency logic in handlers.rs hold up under inspection. No new defects found beyond what prior review rounds already surfaced and disposed of.
There was a problem hiding this comment.
Reviewed the full diff for PR #286 (gh pr diff 286, since the local checkout had no accessible merge-base with main): the new chunk_split.rs splitter, the PriorState/ChunkResubmission idempotency machinery in handlers.rs, the new Db::get_episodes_by_chunk_id/remove_episodes_by_uuids helpers, telemetry, docs, and the updated/added tests in ipc_parity.rs and tier1c_deletion.rs.
The implementation matches the PR description closely: splitting is lossless (verified by tests), the source_description-suffix lineage-reconstruction logic correctly distinguishes never-split vs. split shapes and guards against several edge cases (chunk_ids shaped like #i/N suffixes, foreign knowledge_add_episode name collisions, source_file renames), and the no-op/replace paths are well covered by tests including a newly-added reverse-threshold-crossing case. The known TOCTOU race between the prior-state lookup and the eventual ingest/delete — the dominant concern across this PR's many prior review threads — is unchanged in this final commit's diff beyond adding an explicit #288 cross-reference in the code comment, and is already tracked/accepted per the maintainer's confirmation on an existing thread. The three specific fixes called out in this commit's message (renaming the shadowed unit_count local to expected_n, the #288 comment cross-link, and the new reverse-threshold-crossing test) are all present and correct. No new defects were found.
There was a problem hiding this comment.
Reviewed the full knowledge_process_chunk split/idempotency implementation (chunk_split.rs, handlers.rs's reconstruct_prior_chunk_text/handle_knowledge_process_chunk, db.rs's new episode-lookup/delete-by-uuid helpers, the updated tests, and the ADR/telemetry docs). Note: this checkout has no main ref or history available (single squashed commit, detached HEAD, no remote), so I read the current state of the changed files directly rather than diffing against main.
This PR has already been through an extensive review cycle — 30+ threads, the large majority resolved with either code fixes or accepted-tradeoff replies (the TOCTOU/concurrency gap is consistently disclosed in the ADR, README, and tool description, and tracked as #288). I verified the latest commit's two targeted fixes are present and correct: the unit_count→expected_n rename in reconstruct_prior_chunk_text (no longer shadows the response-level unit_count), the #288 cross-reference added to the TOCTOU code comment, and the new test_knowledge_process_chunk_replace_crosses_threshold_reverse test, which correctly exercises resubmitting a previously-split chunk_id with newly below-threshold text and asserts the full split lineage is replaced by a single fresh episode.
The splitter's invariants (unit length ≤ threshold, lossless concatenation, char-boundary safety) check out against their test coverage, and the lineage-reconstruction parsing (#i/N suffix handling, foreign-row exclusion, Anomalous fallback) is sound and matches its documented design. I found no new defects beyond what prior threads already track.
There was a problem hiding this comment.
Reviewed the current head commit (cae7602), which is the tail end of PR #286's already-lengthy review cycle — the repository is a shallow single-commit clone so I could not diff directly against main; instead I inspected crates/core/src/handlers.rs (the knowledge_process_chunk handler, reconstruct_prior_chunk_text, PriorState/ChunkResubmission), crates/core/src/chunk_split.rs, crates/core/src/db.rs's episode-deletion helpers, docs/telemetry.md, and the relevant tests in ipc_parity.rs/tier1c_deletion.rs.
This commit's own changes per its message — renaming the local unit_count to expected_n in reconstruct_prior_chunk_text to stop shadowing the response-level field, cross-linking #288 directly in the TOCTOU code comment, and adding test_knowledge_process_chunk_replace_crosses_threshold_reverse covering the previously-untested split→single replace direction — are all correctly implemented, consistent with the surrounding code, and the new test exercises the case it claims to. The extensive set of prior review threads on this PR (TOCTOU races on chunk_id resubmission, partial-split orphan accumulation, telemetry unit_count semantics, foreign-name-collision residual gap, delimiter-guard cross-referencing, unbounded splitter cost) all remain accurately documented and either resolved or explicitly accepted as tracked limitations (#288) — nothing in this final commit reopens or regresses any of them. No new defects found.
There was a problem hiding this comment.
Reviewed the core changes for issue #284: the chunk_split module (whitespace-preferred splitter with hard-cut fallback), the knowledge_process_chunk idempotency/resubmission logic in handlers.rs (PriorState/ChunkResubmission reconstruction, no-op/replace paths, telemetry emission), the source_description encode/decode convention, db.rs's episode lookup/delete helpers, and the associated tests, ADR, and telemetry docs.
Note: this checkout is a single-commit shallow clone with no main ref available locally, so I reviewed the current state of the changed files directly (identified via the PR description) rather than a literal git diff main...HEAD.
The code is consistent with the extensive design rationale documented inline and in ADR-0284, and the latest commit already addressed the previously-raised findings (unit_count field-name shadowing, TOCTOU comment cross-referencing #288, and a new reverse-direction threshold-crossing test). I did not find any new correctness, security, or design issues beyond what prior review rounds already surfaced and either fixed or explicitly accepted as tracked limitations (the TOCTOU race deferred to #288, the pathological-input splitter cost, the residual source_description collision risk). No new findings to report.
There was a problem hiding this comment.
Reviewed the chunk-splitting and chunk_id resubmission-idempotency implementation (chunk_split.rs, the handle_knowledge_process_chunk handler and its PriorState/ChunkResubmission reconstruction logic in handlers.rs, the new Db helpers in db.rs, the MCP tool description, README/telemetry docs, and the ADR), plus the corresponding test changes in ipc_parity.rs and tier1c_deletion.rs. This PR head is a follow-up commit ("Address remaining automated-review findings on PR #286"), and the code, tests, and docs are consistent with the fixes described against the prior review threads (the encoder/decoder cross-reference comment, the remove_episodes_by_uuids invariant comment, and the corrected unit_count telemetry-doc wording are all present and accurate). The splitter is lossless and its whitespace-scan/hard-cut/zero-max_chars edge cases are covered by targeted unit tests; the idempotency reconstruction correctly distinguishes never-split vs. split lineages and falls back to Anomalous→replace on any inconsistency. The workspace builds cleanly and the chunk_split tests pass. The well-documented cross-request TOCTOU race (read lock dropped before ingest/delete) is already covered at length by prior threads and explicitly accepted/tracked as #288, so it isn't re-raised here. I did not find any new defects introduced by the current diff.
There was a problem hiding this comment.
Reviewed the knowledge_process_chunk splitting/idempotency implementation (crates/core/src/chunk_split.rs, crates/core/src/handlers.rs), the PriorState reconstruction/encoder-decoder logic for source_description, the DB-layer deletion/lookup helpers (crates/core/src/db.rs), the telemetry event, and the updated tests (ipc_parity.rs, tier1c_deletion.rs) plus docs (README, telemetry.md, ADR-0284). Note: this checkout has no main ref or parent commit reachable via git (single detached commit, no remote), so I reviewed the current state of the changed files directly rather than via git diff main...HEAD.
The splitter is lossless and correctly bounded, with tested edge cases (unbreakable tokens, multibyte chars, zero-max-chars). The idempotency reconstruction logic (reconstruct_prior_chunk_text) correctly handles never-split vs. split lineages, rejects inconsistent/incomplete unit sequences as Anomalous (safe self-healing replace), and the encoder/decoder convention for source_description is now cross-referenced at both sites per the prior review round. The delete-after-successful-ingest ordering avoids leaving a chunk_id with zero episodes on ingest failure. The latest commit's specific fixes (renaming the shadowing unit_count local to expected_n, cross-linking #288 in the code comment, and the new reverse-threshold-crossing replace test) all check out correctly against the code and match the PR description.
I did not find any new defects beyond the concurrency (TOCTOU), partial-split-failure, and delimiter-collision issues already extensively discussed and resolved/accepted in prior review threads on this PR.
There was a problem hiding this comment.
Reviewed the full knowledge_process_chunk split/idempotency implementation (chunk_split.rs, the PriorState/ChunkResubmission machinery and handler body in handlers.rs), the new db.rs query/delete helpers, the ADR, README/telemetry doc updates, tools.rs descriptions, and the new/renamed tests in ipc_parity.rs and tier1c_deletion.rs. This PR has already been through many review rounds; the splitter's whitespace-scan termination/perf characteristics, the encoder/decoder source_description convention, the foreign-name-collision guard, the mid-split partial-failure self-healing story, and the well-documented per-chunk_id concurrency gap (tracked as #288) are all sound and consistent with their accompanying doc comments, and no new defect in the current diff undermines any of those previously-resolved threads. Verified the lossless-concatenation invariant holds end-to-end (episode content is stored verbatim from the unit text), that remove_episodes_by_uuids/get_episodes_by_chunk_id scope correctly, and that the noop/replace/split response-shape branches in handle_knowledge_process_chunk match what the tests and docs claim. No new correctness, security, or design issues found.
There was a problem hiding this comment.
Reviewed the diff between PR #286's head (cae7602) and its actual base (552be21, resolved via gh pr view) — crates/core/src/handlers.rs (the knowledge_process_chunk rewrite: PriorState/ChunkResubmission reconstruction, split-ingest loop, deferred replace-delete), crates/core/src/chunk_split.rs (the whitespace-preferred splitter), the new Db methods (get_episodes_by_chunk_id, remove_episodes_by_uuids), the ChunkTextOversized telemetry event, tool-description/README/ADR/telemetry docs, and the test suite (ipc_parity.rs, tier1c_deletion.rs).
The implementation matches the PR description closely: reconstruction correctly discriminates never-split vs. split lineages via the source_description suffix convention (including the chunk_id-looks-like-a-split-suffix edge case, which is explicitly tested), the encoder/decoder split now cross-references itself per an earlier review round, the replace-delete only runs after a successful fresh ingest (so a failed re-ingest never zeroes out a chunk_id), remove_episodes_by_uuids is scoped only to caller-derived lineage UUIDs, and the splitter's invariants (max-chars bound, lossless concatenation, no infinite loop) hold under inspection and are exercised by targeted unit tests. The extensive prior review threads on this PR (TOCTOU races on the lock-drop-then-ingest sequence, mid-split partial-failure orphaning, the foreign-source_description collision residual gap, the unbounded-request-size DoS surface, and unit_count's pre-ingest-intent semantics) are all either fixed in this diff or explicitly accepted/tracked (#288) with reasoned replies from the author; I did not find any new defect introduced by those fixes, nor any additional correctness issue in the code paths they touch. No new findings to report.
There was a problem hiding this comment.
Reviewed the chunk_split.rs splitter, the knowledge_process_chunk idempotency/reconstruction logic in handlers.rs, the DB helpers it relies on (get_episodes_by_chunk_id, remove_episodes_by_uuids), the updated tests (ipc_parity.rs, tier1c_deletion.rs), and the doc/telemetry/tool-description updates. (The checked-out worktree is a shallow single-commit clone with no main ref or remote available, so this review is based on reading the current state of the changed files rather than a git diff against base.)
The implementation is unusually well-documented and defensively tested: the splitter is lossless and char-boundary-safe with dedicated multibyte/hard-cut/zero-threshold tests, and the lineage-reconstruction logic explicitly handles foreign name collisions, chunk_ids shaped like split suffixes, mixed split/non-split states, and threshold-crossing resubmissions, each with a regression test. One real gap: the code itself documents (and the PR links to issue #288 for) a TOCTOU race where the idempotency read lock is dropped before the fresh ingest's own write lock is acquired, so two concurrent resubmissions of the same never-before-seen chunk_id can both proceed as fresh inserts and produce duplicate/divergent episodes. It's called out in-code as a known, tracked limitation rather than an oversight, but it's a real correctness gap worth surfacing explicitly in review.
| let chunk_id_lookup = chunk_id.clone(); | ||
| let group_id_lookup = group_id.clone(); | ||
| let chunk_text_for_compare = chunk_text.clone(); | ||
| let read_guard = state.write_lock.read().await; |
There was a problem hiding this comment.
Documented (KNOWN LIMITATION, tracked in #288) TOCTOU race: the read lock covering the idempotency lookup (prior_state) is dropped at line 684, before the fresh-ingest path acquires its own write lock (inside episode::add_episode for the non-split case, or per-unit in the split loop). Two concurrent knowledge_process_chunk calls for the same never-before-seen chunk_id — or two racing resubmissions — can both observe PriorState::None/the same prior state and both proceed to insert, producing duplicate or divergent episode sets for one chunk_id until a later resubmission self-heals via the Anomalous path. This is real production-impacting behavior for any caller that retries concurrently (e.g. a crashed client retried by an orchestrator while the original request is still in flight), not just a theoretical edge case.
There was a problem hiding this comment.
I could not diff against main in this environment (the checkout is a single shallow commit with no main ref or reachable parent), so this review is based on reading the current state of the changed subsystem directly: crates/core/src/handlers.rs (the knowledge_process_chunk/knowledge_delete_chunk_episode idempotency and split logic, including PriorState/ChunkResubmission reconstruction), crates/core/src/chunk_split.rs, and the new test in crates/core/tests/ipc_parity.rs. This commit's stated scope is three small follow-ups from the prior review pass — renaming a shadowing local (unit_count → expected_n), cross-linking issue #288 directly in the TOCTOU code comment, and adding a reverse-direction replace test (split → below-threshold) — and all three are correctly and consistently implemented; the rename doesn't introduce any other shadowing, the comment cross-reference is accurate, and the new test correctly asserts that a shrinking resubmission fully replaces every prior split unit with a single fresh episode.
The rest of the split/idempotency design (lock scoping and the documented TOCTOU race, partial-split orphan handling, the source_description encoder/decoder convention, unit_count's pre-ingest-intent semantics, and remove_episodes_by_uuids' trust-the-caller contract) matches what's already been extensively covered across prior review threads on this PR, all resolved with either code fixes or accepted-tradeoff responses that hold up on re-reading. I found no new defects introduced by this commit and no previously-unraised issues in the surrounding code.
Closes #284
Summary
This PR delivers issue #284's full scope, and in doing so subsumes #282's never-implemented baseline (advisory threshold, warning response, telemetry) as an in-scope prerequisite — Research found #282 had a circular
fabrik:blockeddependency on #284's closure, so implementing #282's already-published Plan design here breaks that deadlock. #282 becomes redundant and should be closed once this merges.What changed in
knowledge_process_chunkchunk_textaboveLCG_CHUNK_TEXT_ADVISORY_MAX_CHARS(default 8,000 chars,chars().count()) is now split into threshold-sized units via a new whitespace-preferred, hard-cut-fallback splitter (crates/core/src/chunk_split.rs), rather than silently accepted as a single low-yield episode. Splitting is lossless: every unit's char count ≤ threshold, and concatenating all units reproduces the original text exactly.chunk_id, distinct unit index. Every split unit is ingested as its own episode sharing the caller'schunk_id(Episodic.namestays untouched, soremove_episodes_by_chunk_id's exact-match deletion is unaffected); the unit index lives only insource_description("{source_file}:{chunk_id}#{i}/{N}").chunk_textno-ops (skips extraction, returns existing episode UUID(s) withidempotent: true) to avoid reintroducing LLM-extraction nondeterminism on a byte-identical retry. Differentchunk_textdeletes the prior episode(s) and re-ingests, reporting what was deleted viareplaced_uuids. An unparseable or partial prior state (e.g. a mid-split failure's leftovers) is treated as a mismatch and replaced — which is also how a failed split self-heals on retry, with no separate rollback logic needed.episode_uuids/unit_count/warningfor a split;idempotent: truefor a no-op;replaced_uuidsfor a replace. All new shapes are additive to distinct cases, not silent changes to the existing one.ChunkTextOversizedevent fires wheneverchunk_textexceeds the threshold, regardless of outcome.Behavior changes (called out per FR-009/SC-005)
test_knowledge_process_chunk_duplicate_chunk_id(ipc_parity.rs) flipped fromassert_ne!toassert_eq!— resubmitting an identicalchunk_id+chunk_textpair is now idempotent, not a second unrelated episode.delete_chunk_episode_all_revisions(tier1c_deletion.rs) assumed resubmission always "appends a revision" (2 episodes). Renamed todelete_chunk_episode_after_idempotent_resubmission; now asserts 1 episode, matching the new no-op semantic.chunk_idcan now map to multiple episodes as designed behavior (via splitting), not just as a delete-path curiosityremove_episodes_by_chunk_idhappened to tolerate.Docs
README.md: new Ingestion size-contract and resubmission-idempotency paragraphs;LCG_CHUNK_TEXT_ADVISORY_MAX_CHARSadded to the env var table.docs/telemetry.md: newchunk_text_oversizedevent section.crates/service/src/mcp/tools.rs:knowledge_process_chunk/knowledge_delete_chunk_episodetool descriptions updated.docs/adr/0284-chunk-splitting-and-chunk-id-idempotency.md: new ADR documenting thesource_descriptionencoding convention and the content-comparison idempotency design, cross-referencing ADR-0015, ADR-0038, ADR-0046, ADR-0047.Test plan
cargo fmt --all— cleancargo test(full workspace, debug profile) — 803 passed, 8 ignored, 0 failedcargo clippy --all-targets -- -D warnings— no issuescrates/core/tests/ipc_parity.rsknowledge_delete_chunk_episode) re-verified against split chunkscargo test --release(integration tests requiring release-mode linking) has not been run locally per this repo's stated local-verification budget — this is CI's job.