Repository navigation
feat(nodes): add text Chunker pipeline node - #897
Merged
dsapandora merged 19 commits intoSep 8, 2026
Merged
Conversation
nihalnihalani
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
May 15, 2026 20:34
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a Text Chunker node: three chunking strategies (recursive, sentence, token), IGlobal lifecycle for strategy initialization, per-instance chunking and emission with chunk metadata, service configuration and requirements, and comprehensive tests. ChangesText Chunker Node Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant IGlobal
participant IInstance
participant ChunkingStrategy
Client->>IGlobal: validateConfig()
Note over IGlobal: probe token deps if selected
Client->>IGlobal: beginGlobal()
IGlobal->>ChunkingStrategy: new RecursiveCharacterChunker/SentenceChunker/TokenChunker
ChunkingStrategy-->>IGlobal: strategy instance
Client->>IInstance: open(entry)
IInstance->>IInstance: chunkId = 0
loop process documents
Client->>IInstance: writeDocuments(documents)
IInstance->>IInstance: coerce to Doc schema
IInstance->>ChunkingStrategy: chunk(text)
ChunkingStrategy-->>IInstance: list of chunks with metadata
IInstance->>IInstance: copy Doc per chunk, annotate metadata
IInstance->>Client: instance.writeDocuments(chunks)
end
Client->>IGlobal: endGlobal()
IGlobal->>IGlobal: strategy = None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/chunker/chunker_strategies.py`:
- Around line 129-145: The chunking logic in chunk() prepends overlap to raw
chunks produced by _split_text(), causing final chunk_text to exceed chunk_size
(e.g., raw_chunks from _split_text() are already <= chunk_size but adding
chunk_overlap makes them ~chunk_size+chunk_overlap); fix by ensuring final
chunk_text is truncated to chunk_size after prefixing overlap (or adjust overlap
to not exceed remaining space): locate chunk() where raw_chunks, overlap_text,
and chunk_text are composed and enforce that len(chunk_text) <= self.chunk_size
(trim the prefix or suffix as needed) and recompute start_char/end_char
accordingly so indexes match the trimmed chunk.
- Around line 82-110: The split routine (_split_text) currently drops the
separator when candidate overflows; fix it so boundary separators are preserved
by attaching the separator to one of the emitted chunks: when you decide to
finalize and append current (in the overflow branch where you do
chunks.append(current)), append the separator to that chunk first (e.g., append
current + separator), or alternatively, when starting the next chunk keep the
separator by prefixing part with separator before recursing/assigning it to
current; ensure this change is applied in the overflow branch that handles
len(candidate) > self.chunk_size and the case where len(part) <=
self.chunk_size, and keep use of variables parts, separator, current, candidate,
chunks, self.chunk_size and remaining_separators intact so start_char/end_char
align with the original text.
In `@nodes/src/nodes/chunker/IGlobal.py`:
- Around line 79-96: The branch logic currently silently falls back to
RecursiveCharacterChunker for any unknown strategy_name; change it so
'recursive' is an explicit elif and raise a clear exception (e.g., ValueError)
for any unrecognized strategy_name. Update the conditional that sets
self.strategy (the block that checks strategy_name and instantiates
TokenChunker, SentenceChunker, and RecursiveCharacterChunker) to add an explicit
elif strategy_name == 'recursive' path and an else that raises an error
referencing the invalid strategy_name so typos or stale configs fail fast.
In `@nodes/src/nodes/chunker/IInstance.py`:
- Around line 164-167: The current branch only calls self.preventDefault() when
chunked_docs is truthy, so if no chunks are produced the original batch is not
suppressed; update the method in IInstance (the block using chunked_docs and
self.instance.writeDocuments) to always call self.preventDefault() after
handling the incoming batch — keep the writeDocuments call conditional on
chunked_docs but move or add a call to self.preventDefault() so it executes
unconditionally (ensuring the chunker always consumes the original batch even
when zero chunks are emitted).
In `@test/nodes/test_chunker.py`:
- Around line 395-400: The test currently only asserts a bound on
call_counts['decode'] but can miss implementations that still decode large
prefix slices; modify the test around the decode call tracking (the mock that
increments call_counts['decode']) to also record the input token lengths (e.g.,
push len(tokens) into a list like decode_lengths), then add an assertion after
running the chunker that no decode call was invoked with a large-prefix length
(for example assert max(decode_lengths) <= max_chunk_len or assert all(l <=
threshold for l in decode_lengths)), and keep the existing linear call-count
check; reference the existing names call_counts, decode (mock), chunks, and
tokens when adding the length tracking and the new assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aad80c4d-79fa-4ca7-b14f-d2b9b077b9e6
📒 Files selected for processing (7)
nodes/src/nodes/chunker/IGlobal.pynodes/src/nodes/chunker/IInstance.pynodes/src/nodes/chunker/__init__.pynodes/src/nodes/chunker/chunker_strategies.pynodes/src/nodes/chunker/requirements.txtnodes/src/nodes/chunker/services.jsontest/nodes/test_chunker.py
nihalnihalani
force-pushed
the
feat/chunker-node-clean
branch
from
May 15, 2026 20:46
dae8bf1 to
502fad2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/chunker/chunker_strategies.py`:
- Around line 337-341: The fallback decode currently calls
encoder.decode(chunk_tokens, errors='replace') which tiktoken.decode does not
accept and will raise TypeError; update the exception handling in the chunking
logic that assigns chunk_text so the fallback does not pass an unsupported
errors kwarg—either call encoder.decode(chunk_tokens) without errors, or iterate
chunk_tokens and decode each token safely (e.g., using
encoder.decode_single_token_bytes or per-token decode with try/except) and join
the results, or log the decode failure and return an empty string; adjust the
code around encoder.decode and chunk_tokens (and any variable named chunk_text)
accordingly.
In `@nodes/src/nodes/chunker/IInstance.py`:
- Around line 94-98: The DocMetadata construction in IInstance (calling
DocMetadata(**metadata_data)) can raise TypeError if metadata_data contains
unknown keys; update the code in the method where metadata_data is built to
either (a) filter metadata_data to only the fields accepted by DocMetadata
before calling DocMetadata(**metadata_data) using DocMetadata.__annotations__ or
a known field list, or (b) call DocMetadata.model_validate(metadata_data) /
DocMetadata.parse_obj (whichever validation API is available) to tolerate extra
fields; ensure you still set defaults for 'objectId' and 'chunkId' on the
filtered/validated data and return the same tuple structure.
In `@nodes/src/nodes/chunker/services.json`:
- Around line 54-65: The schema entries "chunker.chunk_size" and
"chunker.chunk_overlap" currently use "type": "number" which allows fractional
and negative values; change "chunker.chunk_size" to "type": "integer" with
"minimum": 1 and change "chunker.chunk_overlap" to "type": "integer" with
"minimum": 0 to ensure whole non-negative counts. Also add a runtime/config
validation (or a JSON Schema cross-field rule if your validator supports it) to
enforce chunker.chunk_overlap < chunker.chunk_size so overlap cannot equal or
exceed the chunk size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33af72ec-c33b-4f85-a558-f5695936d2e1
📒 Files selected for processing (7)
nodes/src/nodes/chunker/IGlobal.pynodes/src/nodes/chunker/IInstance.pynodes/src/nodes/chunker/__init__.pynodes/src/nodes/chunker/chunker_strategies.pynodes/src/nodes/chunker/requirements.txtnodes/src/nodes/chunker/services.jsontest/nodes/test_chunker.py
Collaborator
Author
|
Heads-up on CI: the Ruff check is currently red on this PR because three pre-existing files on develop ( I've opened #900 ( This PR's code-level CI is otherwise fully green: Build / Ubuntu, Build / Windows, Build / macOS, and gitleaks all pass. |
nihalnihalani
force-pushed
the
feat/chunker-node-clean
branch
from
May 15, 2026 22:08
502fad2 to
09d1c46
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
nodes/src/nodes/chunker/IGlobal.py (1)
79-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on unknown
strategyvalues.The current
elsesilently defaults to recursive. A typo in config can run the wrong algorithm without surfacing an error.💡 Suggested fix
if strategy_name == 'token': self.strategy = TokenChunker( chunk_size=chunk_size, chunk_overlap=chunk_overlap, encoding_name=encoding_name, ) elif strategy_name == 'sentence': self.strategy = SentenceChunker( chunk_size=chunk_size, chunk_overlap=chunk_overlap, ) - else: - # Default to recursive character chunker + elif strategy_name == 'recursive': self.strategy = RecursiveCharacterChunker( chunk_size=chunk_size, chunk_overlap=chunk_overlap, ) + else: + raise ValueError(f'Unknown chunking strategy: {strategy_name}')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/chunker/IGlobal.py` around lines 79 - 96, The strategy selection currently falls through to RecursiveCharacterChunker for any unrecognized strategy_name; instead, validate strategy_name when building the strategy (the block that sets self.strategy) and raise a clear exception if it is not one of the supported values ('token' or 'sentence'); keep TokenChunker and SentenceChunker branches as-is and remove the silent defaulting to RecursiveCharacterChunker (or make RecursiveCharacterChunker explicit only for a supported value), referencing the variables/class names strategy_name, self.strategy, TokenChunker, SentenceChunker, and RecursiveCharacterChunker so the caller gets an immediate error on typos or unsupported strategies.nodes/src/nodes/chunker/chunker_strategies.py (3)
129-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOverlap prefix can violate configured
chunk_size.
rawchunks are already bounded, then overlap is prepended unconditionally, so emitted chunks can exceedself.chunk_size.💡 Suggested fix (cap overlap to remaining budget)
for i, raw in enumerate(raw_chunks): - # Compute overlap prefix from the previous chunk - if i > 0 and self.chunk_overlap > 0: - prev = raw_chunks[i - 1] - overlap_text = prev[-self.chunk_overlap :] - chunk_text = overlap_text + raw - else: - chunk_text = raw + overlap_len = 0 + if i > 0 and self.chunk_overlap > 0: + prev = raw_chunks[i - 1] + overlap_len = min(self.chunk_overlap, max(0, self.chunk_size - len(raw))) + overlap_text = prev[-overlap_len:] if overlap_len > 0 else '' + chunk_text = overlap_text + raw + else: + chunk_text = raw @@ - if i > 0 and self.chunk_overlap > 0: - overlap_start = max(0, start_char - self.chunk_overlap) + if overlap_len > 0: + overlap_start = max(0, start_char - overlap_len) start_char = overlap_start🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/chunker/chunker_strategies.py` around lines 129 - 145, The code unconditionally prepends full overlap_text to raw, which can make chunk_text exceed self.chunk_size; change the logic in the chunk assembly (the block using raw_chunks, i, raw, chunk_text and self.chunk_overlap) to compute overlap_len = min(self.chunk_overlap, max(0, self.chunk_size - len(raw))) and then take overlap_text = prev[-overlap_len:] (only if overlap_len > 0) before forming chunk_text = overlap_text + raw; also update the overlap_start adjustment (used to compute start_char) to subtract overlap_len rather than self.chunk_overlap so start_char/end_char remain consistent with the actual prepended bytes.
97-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve boundary separators in overflow splits.
When a candidate overflows, the separator between
currentandpartis dropped. That loses source characters and makes chunk span metadata non-faithful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/chunker/chunker_strategies.py` around lines 97 - 107, The overflow branch in _split_text drops the separator between current and part, losing characters and breaking chunk spans; when len(part) > self.chunk_size and you recurse (sub_chunks = self._split_text(part, remaining_separators)), preserve the boundary separator by including it in the recursive input or by prefixing the first sub_chunk with that separator before extending chunks. Update _split_text to determine the separator string that was used between current and part (from the separators sequence) and either prepend it to part when calling _split_text(part, remaining_separators) or insert it at the front of the first element in sub_chunks, then extend chunks and reset current = ''. Ensure this uses the existing symbols current, part, remaining_separators, chunks, and self.chunk_size so span metadata remains faithful.
337-341:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback decode path calls unsupported
tiktokenAPI.Line 340 and Line 371 pass
errors='replace'toencoder.decode(...); that can raiseTypeErrorin the exception path and fail chunking entirely.💡 Suggested fix (byte-safe fallback)
try: chunk_text = encoder.decode(chunk_tokens) except Exception: - chunk_text = encoder.decode(chunk_tokens, errors='replace') + chunk_bytes = b''.join(encoder.decode_single_token_bytes(tok) for tok in chunk_tokens) + chunk_text = chunk_bytes.decode('utf-8', errors='replace') @@ try: step_text = encoder.decode(step_tokens) except Exception: - step_text = encoder.decode(step_tokens, errors='replace') + step_bytes = b''.join(encoder.decode_single_token_bytes(tok) for tok in step_tokens) + step_text = step_bytes.decode('utf-8', errors='replace')Does Python tiktoken Encoding.decode accept an `errors` keyword argument?Also applies to: 369-372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/chunker/chunker_strategies.py` around lines 337 - 341, The fallback path should not call encoder.decode(..., errors='replace') because tiktoken's Encoding.decode doesn't accept an errors kwarg; instead, catch the original exception and build chunk_text by decoding tokens individually (call encoder.decode([token]) inside a loop), substituting a replacement character (e.g. '\uFFFD') for any token that raises when decoded, then join those parts; apply this change for the occurrences that set chunk_text (the encoder.decode fallback around chunk_text and the similar block at lines 369-372) so both use the per-token safe decode strategy rather than relying on an unsupported errors parameter.nodes/src/nodes/chunker/IInstance.py (1)
164-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways consume the original batch after chunking attempt.
preventDefault()is currently conditional. If no chunks are emitted, original documents can continue unchunked.💡 Suggested fix
if chunked_docs: debug(f'Chunker emitting {len(chunked_docs)} chunks') self.instance.writeDocuments(chunked_docs) - return self.preventDefault() + return self.preventDefault()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/chunker/IInstance.py` around lines 164 - 167, The current logic only calls self.preventDefault() when chunked_docs is truthy, allowing the original documents to continue unchunked; fix by always consuming the original batch after attempting to chunk: keep the self.instance.writeDocuments(chunked_docs) call inside the if chunked_docs block, then unconditionally call and return self.preventDefault() after that block (so chunking still writes when present, but the original batch is always prevented/consumed). Reference: chunked_docs, self.instance.writeDocuments, and self.preventDefault().nodes/src/nodes/chunker/services.json (1)
54-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse integer schema types for chunk counts.
chunk_size/chunk_overlapare count fields;"number"allows fractional values that get silently coerced later.💡 Suggested schema tightening
"chunker.chunk_size": { - "type": "number", + "type": "integer", "title": "Chunk size", "description": "Maximum size of each chunk (characters for recursive/sentence, tokens for token strategy)", - "default": 1000 + "default": 1000, + "minimum": 1 }, "chunker.chunk_overlap": { - "type": "number", + "type": "integer", "title": "Chunk overlap", "description": "Number of characters or tokens to overlap between consecutive chunks", - "default": 200 + "default": 200, + "minimum": 0 },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/chunker/services.json` around lines 54 - 65, The schema uses "number" for chunker.chunk_size and chunker.chunk_overlap which permits fractional values; change their JSON Schema "type" to "integer" (and optionally add "minimum": 0) so these count fields only accept whole non-negative integers; update the entries named "chunker.chunk_size" and "chunker.chunk_overlap" in services.json accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@nodes/src/nodes/chunker/chunker_strategies.py`:
- Around line 129-145: The code unconditionally prepends full overlap_text to
raw, which can make chunk_text exceed self.chunk_size; change the logic in the
chunk assembly (the block using raw_chunks, i, raw, chunk_text and
self.chunk_overlap) to compute overlap_len = min(self.chunk_overlap, max(0,
self.chunk_size - len(raw))) and then take overlap_text = prev[-overlap_len:]
(only if overlap_len > 0) before forming chunk_text = overlap_text + raw; also
update the overlap_start adjustment (used to compute start_char) to subtract
overlap_len rather than self.chunk_overlap so start_char/end_char remain
consistent with the actual prepended bytes.
- Around line 97-107: The overflow branch in _split_text drops the separator
between current and part, losing characters and breaking chunk spans; when
len(part) > self.chunk_size and you recurse (sub_chunks = self._split_text(part,
remaining_separators)), preserve the boundary separator by including it in the
recursive input or by prefixing the first sub_chunk with that separator before
extending chunks. Update _split_text to determine the separator string that was
used between current and part (from the separators sequence) and either prepend
it to part when calling _split_text(part, remaining_separators) or insert it at
the front of the first element in sub_chunks, then extend chunks and reset
current = ''. Ensure this uses the existing symbols current, part,
remaining_separators, chunks, and self.chunk_size so span metadata remains
faithful.
- Around line 337-341: The fallback path should not call encoder.decode(...,
errors='replace') because tiktoken's Encoding.decode doesn't accept an errors
kwarg; instead, catch the original exception and build chunk_text by decoding
tokens individually (call encoder.decode([token]) inside a loop), substituting a
replacement character (e.g. '\uFFFD') for any token that raises when decoded,
then join those parts; apply this change for the occurrences that set chunk_text
(the encoder.decode fallback around chunk_text and the similar block at lines
369-372) so both use the per-token safe decode strategy rather than relying on
an unsupported errors parameter.
In `@nodes/src/nodes/chunker/IGlobal.py`:
- Around line 79-96: The strategy selection currently falls through to
RecursiveCharacterChunker for any unrecognized strategy_name; instead, validate
strategy_name when building the strategy (the block that sets self.strategy) and
raise a clear exception if it is not one of the supported values ('token' or
'sentence'); keep TokenChunker and SentenceChunker branches as-is and remove the
silent defaulting to RecursiveCharacterChunker (or make
RecursiveCharacterChunker explicit only for a supported value), referencing the
variables/class names strategy_name, self.strategy, TokenChunker,
SentenceChunker, and RecursiveCharacterChunker so the caller gets an immediate
error on typos or unsupported strategies.
In `@nodes/src/nodes/chunker/IInstance.py`:
- Around line 164-167: The current logic only calls self.preventDefault() when
chunked_docs is truthy, allowing the original documents to continue unchunked;
fix by always consuming the original batch after attempting to chunk: keep the
self.instance.writeDocuments(chunked_docs) call inside the if chunked_docs
block, then unconditionally call and return self.preventDefault() after that
block (so chunking still writes when present, but the original batch is always
prevented/consumed). Reference: chunked_docs, self.instance.writeDocuments, and
self.preventDefault().
In `@nodes/src/nodes/chunker/services.json`:
- Around line 54-65: The schema uses "number" for chunker.chunk_size and
chunker.chunk_overlap which permits fractional values; change their JSON Schema
"type" to "integer" (and optionally add "minimum": 0) so these count fields only
accept whole non-negative integers; update the entries named
"chunker.chunk_size" and "chunker.chunk_overlap" in services.json accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f645ca2c-4075-4927-a77c-c82f0093b952
📒 Files selected for processing (10)
nodes/src/nodes/chunker/IGlobal.pynodes/src/nodes/chunker/IInstance.pynodes/src/nodes/chunker/__init__.pynodes/src/nodes/chunker/chunker_strategies.pynodes/src/nodes/chunker/requirements.txtnodes/src/nodes/chunker/services.jsonnodes/src/nodes/llm_vision_gemini/IGlobal.pynodes/src/nodes/llm_vision_gemini/gemini_vision.pynodes/src/nodes/llm_vision_openai/openai_vision.pytest/nodes/test_chunker.py
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
May 15, 2026
Those nodes are tracked in PRs rocketride-org#897 and rocketride-org#899 separately. They were accidentally included on this branch via an older merge (commit 905ab76) and were causing a dynamic-test failure unrelated to the Helm chart. Also restored upstream versions of asset files that were drifted by the same merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
May 15, 2026
Prior run failed on `from rocketlib import normalize_tool_input` — the symbol IS exported from rocketlib/__init__.py line 63/141 on this branch (identical to develop). Same import passes on companion PRs rocketride-org#897/rocketride-org#898/rocketride-org#899/rocketride-org#900 with the same merge of develop, so this is a stale build-cache flake. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Could you please review the new commit? |
stepmikhaylov
previously requested changes
May 19, 2026
stepmikhaylov
left a comment
stepmikhaylov
left a comment
Collaborator
There was a problem hiding this comment.
Change requests
-
Missing
"experimental"capability —nodes/src/nodes/chunker/services.json:5has"capabilities": []. New nodes must include"experimental"(cf.agent_rocketride,tool_exa_search). -
Test file in wrong directory —
test/nodes/test_chunker.pydoes not follow the convention. Tests must be placed undernodes/test/chunker/. -
Mocks for built-in modules
rocketlib,ai.*, anddepends—test/nodes/test_chunker.py:434–521(TestChunkerLifecycle._install_stubs) installssys.modulesstubs forrocketlib,ai,ai.common,ai.common.config,ai.common.schema, anddepends. These are provided by the build interpreter and must not be mocked. -
File-path-based imports bypass interpreter path configuration —
test/nodes/test_chunker.py:21–37usesPath(__file__).resolve().parent.parent.parent / ...andimportlib.util.spec_from_file_location()to locate modules by filesystem path. Import by package name instead; the build interpreter configures paths correctly. -
Dead code:
_pending_docsbuffer —nodes/src/nodes/chunker/IInstance.py:writeDocuments()emits synchronously and never populates_pending_docs, making_flush_documents(), theopen()initializer, andclosing()unreachable.
Collaborator
|
Thank you for the contribution! The chunker node is a solid addition — the strategy implementations are well-structured and the core logic is clean. Please address the change requests below before we can merge. |
Collaborator
Author
Update — addressed @stepmikhaylov + CodeRabbit feedback (pushed bc93386)stepmikhaylov structural items:
CodeRabbit correctness fixes:
|
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
May 19, 2026
- Set capabilities to ["experimental"] consistent with sibling new nodes (PRs rocketride-org#897/rocketride-org#898). - Add a "test" block per docs/README-node-testing.md so the dynamic test runner exercises the node through the real server. The "balanced" profile re-ranks three pre-scored documents and asserts the top result mentions "machine learning". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Following up: the stepmikhaylov structural items and CodeRabbit correctness items have all been addressed in bc93386. Specifically: tests relocated to nodes/test/chunker/test_all.py, capabilities marked experimental, dead code removed, separator preservation in recursive split, overlap capped below chunk_size, safe tiktoken decode fallback without unsupported kwargs, and services.json chunk sizing fields tightened to integer typing with minimums. Two items kept as-is with reasoning: open() retained because chunkId resets per Entry rather than per document, and DocMetadata(**metadata_data) verified safe since DocMetadata uses extra='allow'. CI is green across Ubuntu, macOS ARM64, and Windows. @stepmikhaylov @Rod-Christensen @jmaionchi could you take another look when you get a chance? |
stepmikhaylov
pushed a commit
that referenced
this pull request
May 19, 2026
* fix(deps): update pnpm-lock.yaml for lucide-react dependency * chore: optimize SVG images via ImgBot (2.95% reduction) Automated image optimization by ImgBot. 85 SVG files optimized with total size reduction from 462.69kb to 449.05kb (2.95%). * feat(nodes): add text chunker and hybrid search nodes Three chunking strategies (recursive, sentence, token) with configurable size/overlap. Hybrid search node with BM25 + vector RRF fusion. All CodeRabbit review feedback addressed including critical writeAnswers contract fix, score propagation, RRF validation, and typing modernization. * feat(engine): add Kubernetes Helm chart for production deployment - Add Helm chart with engine deployment, service, HPA, ingress, configmap, secret - Add optional PostgreSQL+pgvector StatefulSet with PVC persistence - Add optional ChromaDB deployment - Configurable resource limits, replicas, probes, TLS, service accounts - Helm test for /ping connectivity verification - Comprehensive values.yaml with production-ready defaults Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(engine): address code review feedback on Helm chart PR - Fix postgres probes to use Helm template values instead of shell expansion - Remove Milvus section (use official subchart instead), clean up NOTES.txt - Change default postgres password to 'changeme' - Add secret checksum annotation for automatic pod restart on secret changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(helm): address CodeRabbit review feedback - Wrap pg_isready probe commands in sh -c for proper env var expansion - Guard HPA metrics list to avoid empty array when both targets unset - Add email field to Chart.yaml maintainers entry Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(helm): address reviewer feedback on Helm chart PR #510 - Add fail guard in HPA when autoscaling enabled without metrics - Add checksum/postgres-secret annotation to both engine deployment and postgres StatefulSet for automatic rollout on credential changes - Add WARNING comments for default Postgres credentials in values.yaml - Fix NOTES.txt NodePort address lookup to filter ExternalIP first - Add securityContext and resource limits to test pod - Document Chroma PVC lifecycle (intentional data persistence) - Add GPU resource/nodeSelector/tolerations documentation in values.yaml - Document HPA limitation for GPU inference workloads - Clarify chart is for community/self-hosted deployments - Add Helm templates to .prettierignore (Go template syntax) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(helm): show autoscaling info in NOTES.txt and harden container security - NOTES.txt now shows HPA min/max replicas when autoscaling is enabled instead of the static replicaCount which is misleading - Set readOnlyRootFilesystem: true in default securityContext - Add /tmp emptyDir volume mount to deployment template so the engine has a writable scratch directory with read-only root filesystem Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(helm): remove bundled Postgres and ChromaDB, add external service examples This chart should not bundle stateful services that are better managed externally. Users should provision PostgreSQL and ChromaDB via managed cloud services or dedicated Helm charts. - Delete postgres-statefulset.yaml and chroma-deployment.yaml templates - Remove postgres/chroma sections from values.yaml - Remove postgres env vars and checksum annotation from deployment.yaml - Remove postgres secret helper from _helpers.tpl - Add deploy/helm/examples/ with external-postgres.yaml, external-chroma.yaml - Add examples/README.md documenting how to use managed services Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs(helm): add architecture docs and GPU limitation warnings - Create deploy/helm/ARCHITECTURE.md with community vs SaaS comparison - Add WARNING comment in hpa.yaml about CPU/memory HPA being unsuitable for GPU inference workloads - Create deploy/helm/examples/keda-gpu-scaling.yaml with KEDA ScaledObject example for GPU-aware autoscaling - Add ARCHITECTURE.md reference in Chart.yaml Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(helm): add GPU support with nodeSelector and tolerations - Add engine.gpu config section (enabled, count, nodeSelector, tolerations) - Conditionally add nvidia.com/gpu resource limit in deployment template - Merge GPU nodeSelector and tolerations with base engine values - Create deploy/helm/examples/gpu-values.yaml with working GPU example Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(helm): add secret validation and update appVersion to 3.1.2 - Add rocketride.validateSecrets template that fails if neither engine.existingSecret nor engine.secrets is configured - Invoke validation at the top of the deployment template - Update Chart.yaml appVersion from 3.1.1 to 3.1.2 to match package.json - Verify health endpoint: /ping on port 5565 is correct (confirmed via Dockerfile.engine EXPOSE and AI web server route registration) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(helm): address post-review findings on Kubernetes Helm chart - Lower engine.replicaCount default 2 → 1 for single-node/lab users; update autoscaling.minReplicas to match; add HA tuning note in comment - Add operator-facing README.md covering install/upgrade/uninstall, existingSecret pattern, examples pointer, and HA tuning guide - Add helm-lint + kubeconform CI jobs to ci.yml, path-filtered to deploy/helm/** via dorny/paths-filter (matches existing ci.yml style) Exec audit: confirmed no exec.command blocks with $(VAR) syntax exist anywhere in deploy/helm/rocketride/templates/ — clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ci): add helm-lint to ci-ok gatekeeper needs helm-lint and helm-changes were added to ci.yml but omitted from the ci-ok aggregator job, meaning helm failures would not block PR merges. Added both to needs[] and updated the echo statement to include their results in the CI OK diagnostic output. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(helm): support external secret rollout bump * style(tests): ruff format chunker and hybrid search tests * chore(helm): remove out-of-scope chunker/search_hybrid scope creep Those nodes are tracked in PRs #897 and #899 separately. They were accidentally included on this branch via an older merge (commit 905ab76) and were causing a dynamic-test failure unrelated to the Helm chart. Also restored upstream versions of asset files that were drifted by the same merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: imgbot[bot] <31301654+imgbot[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Change RequestsTests assume the build interpreter — no fallbacks for vanilla pytest
|
Collaborator
|
@nihalnihalani, Thank you for addressing the previous change requests — please address the ones posted above. |
Collaborator
Author
|
Change requests addressed — requesting re-review. @stepmikhaylov @jmaionchi @Rod-Christensen @kwit75
ruff clean; node tests run under the build interpreter in CI. |
Collaborator
Author
|
@stepmikhaylov — addressed the build-interpreter test points; Build is green again across Ubuntu/Windows/macOS.
|
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
May 21, 2026
The build interpreter provides rocketlib, ai.* and depends (plus the native engLib) at runtime, and CI installs tiktoken/json5. Mirror the validated chunker pattern (PR rocketride-org#897): keep only the nodes/src/nodes sys.path insert and import the context_optimizer package by name. Remove the engLib/depends stubs, the rocketlib/ai/client-python sys.path additions, the stale rocketlib/ai sys.modules purge, and the test-local tiktoken/json5 stub injection. No skips or fallbacks: outside the build interpreter, imports error at collection by design. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
May 21, 2026
Align the search_hybrid node tests with the maintainer's "build interpreter is mandatory" philosophy (mirrors PR rocketride-org#897): - Put nodes/src/nodes on sys.path and import the engine, IInstance and IGlobal by PACKAGE NAME (search_hybrid.hybrid_search / .IInstance / .IGlobal) instead of importlib.util.spec_from_file_location. File-path-based imports bypass interpreter path configuration. - Import the real schema types directly: from ai.common.schema import Answer, Doc, Question, QuestionText. QuestionText replaces the duck-typed _SubQuestion stand-in. - Remove the _HAS_ROCKETLIB / _HAS_AI_SCHEMA find_spec gates, the skipif on TestIGlobalAlphaClamp, and the in-fixture / in-test pytest.skip fallbacks. rocketlib, ai.* and depends are provided by the build interpreter and must not be stubbed; collection/fixture error when absent is the intended behavior. - Keep the rank_bm25 third-party compute-lib mock (nodes/test/mocks/rank_bm25) and its module-scoped install/teardown; it is the only stub, on par with the openai / elasticsearch mocks. - Preserve the writeAnswers single-Answer contract (ans.answer) and the keyword QuestionText(text=...) construction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nihalnihalani
added a commit
to nihalnihalani/rocketride-server
that referenced
this pull request
Jul 7, 2026
Sync PR rocketride-org#897 branch with develop (232 commits behind). No conflicts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a `chunker` pipeline node that splits long documents into smaller chunks suitable for embedding and retrieval. Multiple chunking strategies are bundled: recursive character splitting, sentence-aware splitting, and token-based splitting. Components: - IGlobal — standard lifecycle with depends() for SDK auto-install. - IInstance — receives documents, applies the configured strategy, writes chunked documents to the documents output lane. - chunker_strategies — pluggable strategy implementations. - services.json — strategy profiles with configurable chunk_size and overlap. Resubmission of the chunker portion of closed PR rocketride-org#499 per maintainer feedback: one PR per node, clean branch from develop, no unrelated file changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Set capabilities to ["experimental"] (required for new nodes) and switch chunk_size/chunk_overlap from "number" to "integer" with minimum bounds. Cross-field validation (overlap < size) remains enforced at runtime in IGlobal.beginGlobal and in each strategy constructor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add an explicit elif branch for 'recursive' and raise ValueError when the configured strategy_name is none of recursive/sentence/token. Previously a typo (e.g. 'recurisve') would silently fall through to recursive, masking bad config in production. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fault writeDocuments() emits chunks synchronously and never populated the _pending_docs buffer, so _flush_documents()/closing()/close() were dead helpers that risked re-emitting if anything ever did populate the buffer. Remove them; keep open() solely to reset chunkId per object. Also always call preventDefault() after handling a batch: previously, if every input document produced zero chunks (empty/whitespace), the originals leaked downstream. Switch DocMetadata construction to model_validate() when available; this avoids colliding with DocMetadata.__init__'s positional ``pInstance`` arg and matches the pattern already used for Doc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three correctness fixes in chunker_strategies: 1. RecursiveCharacterChunker._split_text: when a separator-driven emit happens, append the separator to the emitted chunk instead of dropping it. Without this, every overflow silently lost separator characters, causing chunk reconstruction to diverge from the source text. 2. RecursiveCharacterChunker.chunk: cap the prepended overlap to the remaining budget (chunk_size - len(raw)) so an already-full chunk plus overlap can never exceed chunk_size. Use the *applied* overlap when adjusting start_char so offsets stay truthful. 3. TokenChunker: tiktoken's Encoding.decode does not accept an ``errors=`` kwarg (passing one raises TypeError). Replace the bogus fallback with _safe_decode, which falls back to decode_single_token_bytes per token and uses errors='replace' at the bytes -> str boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Match the per-node test convention used by agent_rocketride,
tool_exa_search and local_text_output: a ``test_all.py`` under
``nodes/test/<node>/`` rather than the legacy ``test/nodes/test_*.py``
path.
Drop the sys.modules stubs for rocketlib/ai.common.schema/depends
(maintainer policy: do not mock built-in interpreter modules) and replace
the ``spec_from_file_location`` path imports with normal package imports
(``from chunker.chunker_strategies import ...``) after putting
``nodes/src/nodes`` on sys.path.
The package import is guarded by ``pytest.importorskip('chunker')`` so a
vanilla local pytest run (without the build interpreter that provides
rocketlib) skips cleanly instead of erroring at collection.
Coverage retained: every assertion from the original suite plus three new
regression tests for the fixes in this PR — separator preservation on
overflow, overlap capping under chunk_size, preventDefault on empty
batches, ValueError on unknown strategy_name, and _safe_decode fallback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Strengthen the O(n^2) decode regression guard: track decode input token lengths in the mock encoder and assert max(decoded_lengths) <= chunk_size, in addition to the existing linear call-count assertion. A growing-prefix decode regression can keep the call count linear while inflating individual decode inputs, which the count-only check would miss. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per maintainer review: the build interpreter is mandatory and configures import paths. Remove the sys.path manipulation, pytest.importorskip, and the try/except pytest.skip guards in _import_node_classes()/_import_schema() so a missing module surfaces as a collection error instead of a silent skip.
Removing pytest.importorskip and the try/except pytest.skip guards (per review) so missing build-interpreter modules surface as collection errors instead of silent skips. Restored the nodes/src/nodes sys.path prefix: without it the node package is not importable even under the build interpreter (CI: ModuleNotFound 'chunker'), matching every other node suite (local_text_output, milvus, ...).
The chunker suite's addition perturbs pytest's collection order, which exposed a latent fragility in the shared "prepend nodes/src/nodes" import pattern: `nodes/test/<node>/` is itself importable as `<node>` (it has an `__init__.py`), and a plain `if str(NODES_SRC) not in sys.path` guard does not guarantee the node source is searched *before* the test directory. When another node suite had already appended `nodes/src/nodes` behind the test dir, `import local_text_output` resolved to `nodes/test/local_text_output` (which has no `IInstance`), failing collection with `ModuleNotFoundError: No module named 'local_text_output.IInstance'` on Ubuntu and macOS. Force `nodes/src/nodes` to the front of `sys.path` in both the chunker and local_text_output suites so the real node package always wins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…Type (rocketride-org#1643) Review response (joshuadarron, CHANGES_REQUESTED): - Add co-located chunker.svg icon (services.json referenced a missing asset — broken node-picker render). - Add co-located README.md with the generated schema/dependencies/source block, config fields, and lanes (co-location rule; 106/120 nodes ship one). - Fix overlap silently dropping to 0 on full-size chunks: split now reserves overlap headroom (_split_size = chunk_size - overlap) and reads the overlap from the original text preceding each raw chunk, so overlap is honored on the common prose/hard-split path. Cap the applied overlap by min(chunk_overlap, raw_start, chunk_size - len(raw)) so a trailing separator on a full raw chunk can never push an emitted chunk past chunk_size — the two regression tests (overlap honored on a full-size chunk; no chunk exceeds chunk_size with real '. ' separators, input found by fuzzing) fail on the pre-fix code and pass after. - classType 'transform' -> 'preprocessor' (the established category for text splitters; no other node uses 'transform'). Closes rocketride-org#1643 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…obal sys.path mutation Follow-up to review response, from a fresh-eyes audit of my own fix: - Real invariant bug: the previous cap only trimmed the overlap, so a raw chunk emitted as current+separator could still exceed chunk_size when chunk_overlap was smaller than the trailing separator (e.g. default '\n\n' with overlap 0 or 1 — both schema-allowed). Cap the emitted body at chunk_size (dropping only excess trailing separator chars, never content); current always fits _split_size so no content is lost. 0 invariant violations across 200k fuzz trials with random multi-char separators; full overlap still honored on true full-size chunks. Regression test added for overlap 0/1 with default separators. - Test import: load chunker_strategies.py directly (pure stdlib) instead of importing the chunker package and permanently prepending src/nodes to sys.path at module import; the IGlobal/IInstance tests scope that insertion to a restored context manager. Removes a global import-state side effect that could perturb how sibling node suites resolve same-named test-vs-source packages. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on tests My previous commit's direct-file load of chunker_strategies.py made the module-level strategy classes distinct objects from the ones the chunker package instantiates, so the IGlobal lifecycle tests' isinstance(strategy, SentenceChunker) failed. Restore the working package import (as on 908d3c6, whose chunker tests passed) and keep the three overlap invariant tests. The chunker_strategies.py emit_len size-cap fix is unchanged. Note: the separate image_cleanup collection failure is a pre-existing fragility in upstream test/image_cleanup/test_provenance.py (it assumes 'import image_cleanup' resolves to the source node; the same-named test package can shadow it under xdist), surfaced by adding a node test — not a chunker defect. A hardening fix for that test is proposed separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two small alignments with today's develop: - services.json gained the `documentation` key that 141 of develop's services.json files already carry (bare docs root, placed right after `icon`). Without it the node's Documentation menu entry has no target. - The test module docstring still referred to the `milvus` and `pinecone` suites by their pre-ddf94dd6 names; they are `store_milvus` and `store_pinecone` on develop. Comment-only change, no behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-org#1643) Every node PR merged since July adds a catalog row and a changelog entry; this PR was written before that became the norm. - docs/README-nodes.md: new Document Processing row for `chunker` (documents -> documents). The header counts on lines 13-14 are already stale against develop, so they are deliberately left alone rather than bumped by one on a number that is wrong either way. - CHANGELOG.md: [Unreleased] / ### Added line, alphabetical among the existing **nodes** entries. Note: open PR rocketride-org#2139 removes docs/README-nodes.md as part of the docs revamp. If that lands first this row goes away with the file; nothing else in this PR depends on it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nihalnihalani
force-pushed
the
feat/chunker-node-clean
branch
from
September 4, 2026 07:23
93c165c to
fee9359
Compare
Collaborator
Author
|
@stepmikhaylov @joshuadarron — this PR has been rebased onto today's What changed since your reviews (current head
The July review items remain addressed: co-located On the overlap question with Scope after the rebase is CI: green on the current head AI-assisted (Claude Code), reviewed by the author. |
CONTRIBUTING.md requires the MIT header block on Python sources, and every other file in this node (IGlobal.py, IInstance.py, chunker_strategies.py) already carries it — only the package __init__.py was missing it. Copied verbatim from the canonical form used by 94 of develop's node __init__.py files (e.g. nodes/src/nodes/__init__.py); byte-identical to the block on this node's own IGlobal.py. nodes/test/chunker/__init__.py is deliberately left empty: only 23 of develop's 83 node test-package __init__.py files carry a header, and 44 of them are empty, so the majority convention is no header there. Comment-only change: no behaviour, no imports, no exports touched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
@joshuadarron on your minor AI-assisted (Claude Code), reviewed by the author. |
dsapandora
requested changes
Sep 7, 2026
dsapandora
left a comment
dsapandora
left a comment
Collaborator
There was a problem hiding this comment.
I went looking for the usual chunker defects and did not find them: search_start advances past each raw chunk so repeated text cannot mislocate offsets, the cap at chunk_size drops only surplus separator characters and says so, and the overlap clamp holds at chunk_overlap of 0 and 1. The logic is sound.
The request is about scope, left inline.
…langchain The preprocessor_langchain node already exposes LangChain's RecursiveCharacterTextSplitter through its `default` and `recursive` profiles (nodes/src/nodes/preprocessor_langchain/services.json, langchain.py:186-204), so shipping a second recursive implementation gave the product two ways to do the same thing and left pipeline authors to guess which node to reach for. Narrow this node to the two strategies the engine does not already have: - sentence: stdlib-regex sentence grouping, no third-party dependency and no language-model download (unlike the NLTK/Spacy profiles of General Text). - token: real tiktoken BPE counts, versus the byte-length approximation (`bytes/3`) that General Text labels "Estimated tokens" (langchain.py:50-59). `sentence` becomes the default profile. A stale `strategy: recursive` config now raises at startup naming preprocessor_langchain rather than silently falling back, so existing pipelines fail loudly with a pointer. Docs record why the split exists and what each node is for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A sentence is indivisible in this strategy, so chunk_size is a grouping target rather than a hard cap. With 'sentence' now the default profile, input that carries no sentence-ending punctuation (log lines, CSV rows, minified text, OCR dumps) has no boundary to group on and comes back as a single oversized chunk -- previously the recursive default would have hard-split it. Document the case in the README, route those inputs to the token strategy (which caps unconditionally) or the General Text node, and pin both halves of the behavior in a test so the trade-off stays explicit. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Ready for another look — the latest review's ask is done: the recursive strategy is removed in favor of |
dsapandora
approved these changes
Sep 7, 2026
dsapandora
left a comment
dsapandora
left a comment
Collaborator
There was a problem hiding this comment.
Verified the removal rather than taking it on trust: RecursiveCharacterChunker is gone from chunker_strategies.py, the tests no longer reference it, and services.json now offers sentence and token only.
The part I did not ask for and that makes this land cleanly: strategy: recursive still raises at startup, but with a message naming preprocessor_langchain and its profiles instead of a bare "unknown strategy". An existing .pipe configured against the old shape gets a pointer rather than a dead end. The README carries the same table.
What is left is what the engine genuinely lacked: sentence-boundary splitting with no third-party dependency, and token splitting on the real tiktoken BPE rather than a byte estimate.
Collaborator
|
@stepmikhaylov @joshuadarron — this is still blocked on your July and May reviews. I went through both against the current branch and every point looks addressed, so a re-review from either of you would unblock it. What I checked, in the code rather than the summary: @stepmikhaylov's five: @joshuadarron's: Your other two resolved together. You asked whether this overlapped CI is green, no unresolved threads, branch current with develop. I have approved; yours are the two that still hold it. |
dsapandora
self-requested a review
September 8, 2026 13:55
dsapandora
dismissed stale reviews from stepmikhaylov and joshuadarron
September 8, 2026 14:50
All five verified applied on the current branch: capabilities is ["experimental"], the suite moved to nodes/test/chunker/, no sys.modules stubs remain for rocketlib/ai/depends, no spec_from_file_location imports, and the dead _pending_docs buffer with its unreachable _flush_documents/closing is gone. Dismissing so this can land; reopen if I have read any of them wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
chunkerpreprocessor node (nodes/src/nodes/chunker/) that splits documents on thedocumentslane into one document per chunk, with three strategies: recursive character (separator hierarchy\n\n,\n,.,chunk_sizeis emitted whole, documented in the node README), and token-based (tiktoken, lazy-imported so the other two strategies pull in no dependencies).chunk_overlapis reserved insidechunk_size, so an emitted chunk never exceedschunk_sizeand the configured overlap is still honored on full-size chunks — including the hard-split path. Each chunk carrieschunkId,parentId,chunk_index,start_char,end_charandtotal_chunks;start_char/end_charstay truthful under overlap (test_overlap_honored_on_full_size_chunksassertstext == source[start_char:end_char]for every overlapped chunk). Source metadata is copied per chunk, never shared.chunker.svgicon andREADME.md(with theROCKETRIDE:GENERATED:PARAMSmarkers), aservices.jsonwithclassType: ["preprocessor"],capabilities: ["experimental"]and a livetestblock, plus a 644-line unit suite atnodes/test/chunker/test_all.py. All four Python files carry the MIT header.docs/README-nodes.mdand aCHANGELOG.md[Unreleased] / ### Addedentry.How this differs from the existing
preprocessor_langchain: that node hard-codeschunk_overlap = 0for every splitter it constructs (nodes/src/nodes/preprocessor_langchain/langchain.py:168, passed at:180) and emits no per-chunk character offsets.chunkermakes overlap configurable and recordsstart_char/end_charfor every chunk.Type
Feature (new node). No changes to existing nodes, the test harness, or CI.
Testing
nodes/test/chunker/test_all.py, 48 tests (47 test functions, one parametrized overoverlap in [0, 1]): per-strategy behavior, size/overlap invariants (test_overlap_never_exceeds_chunk_size,test_overlap_stays_within_chunk_size_with_real_separators,test_chunk_never_exceeds_chunk_size_when_overlap_below_separator,test_overlap_honored_on_full_size_chunks), an O(n²) decode-regression guard on the token strategy, edge cases (empty / whitespace-only / single character / no separator), metadata correctness, andIGlobal/IInstancelifecycle.TestRecursiveCharacterChunker,TestSentenceChunker,TestTokenChunker) pass on CPython 3.12.12 (40 passed, 8 deselected) — note the chunker package__init__importsIGlobal, which importsrocketlib, so outside the engine buildchunker.chunker_strategieshas to be pre-imported before collection. The remaining 8 (TestIGlobalLifecycle,TestIInstanceWriteDocuments) importrocketlibandai.common.schemaand fail locally withModuleNotFoundError: No module named 'rocketlib'by design — there are nopytest.skip/importorskipfallbacks; they run under the engine build interpreter in CI.ruff check,ruff format --check(7 files) andnode nodes/scripts/gen-credentials.mjs --check(exit 0, no catalog entry required) were run against this head../builder testpasses — in CI, not locally (no engine build in this checkout). Run 33893136431 on the current headb627bed8ran the full matrix — Ruff, gitleaks, Build on Ubuntu 22.04 / Windows Server 2022 / macOS (ARM64) with thebuilder test --verbose --sequentialstep, Shell API contract — andCI OKcompleted green.Checklist
gitleaks detect --config .gitleaks.toml --log-opts upstream/develop..HEADscans 16 commits, no leaks;gen-credentials.mjs --checkfinds no credential-shaped field (chunker.tokenis a grouping definition, not a field).nodes/src/nodes/chunker/README.mdplus thedocs/README-nodes.mdcatalog row.Sync with develop (2026-09-04)
Rebased onto
upstream/develop@65a2e133a10ed762640ce71a63ef13e8abf6dfc0; the branch is 16 linear commits on top of it (headb627bed8). The branch previously carried two merge-from-develop commits, which the rebase removes (CONTRIBUTING.md:109-116 asks for rebases). Two further commits were dropped:09d1c466chore(format): apply ruff format to llm_vision_* files— a duplicate of the closed PR chore(format): apply ruff format to llm_vision_* files #900. develop already has that formatting via07f78f3e(2026-05-15). This PR now touches nollm_vision_*file at all.93c165cetest: front-load nodes/src/nodes on sys.path in conftest— dropped;nodes/test/conftest.pyis untouched by this PR. Theimage_cleanupcollection error it was aimed at is fixed on develop: the real cause wasnodes/test/cloud_tts/test_cloud_tts.pydoingsys.path.insert(0, <nodes/test root>)at module import time (:25before39bc5090), which shadowednodes/src/nodeson any xdist worker that collectedcloud_ttsfirst — no conftest-level insert could override that.39bc5090(feat(nodes): add tool_pipedrive CRM tool node #1687) replaced that insert with a package-relativefrom ..test_contracts import parse_service_json, addedtest_test_root_is_not_on_sys_path, and gavenodes/test/image_cleanup/test_provenance.pyits own front-of-path insert (:29-32).Also dropped: the
nodes/test/local_text_output/test_instance.pyhunk. develop already has the strongsys.pathinsert this PR was proposing (:14-19, from #1687), so develop's version of that file is kept verbatim.Ported on top:
"documentation": "https://docs.rocketride.org"inservices.json(141 of develop'sservices.jsonfiles carry it, right after"icon"); the test-module docstring's stale suite namesmilvus/pineconeupdated tostore_milvus/store_pineconeafter theddf94dd6rename; thedocs/README-nodes.mdcatalog row and theCHANGELOG.mdentry. The catalog header's "121 node directories → 151 services" count is left untouched — it is already stale on develop (137 node directories), so bumping it here would be unrelated churn.git diff --name-only 65a2e133..HEADis exactly 12 files: the eight undernodes/src/nodes/chunker/, the two undernodes/test/chunker/,docs/README-nodes.mdandCHANGELOG.md(1573 insertions, 0 deletions).Commits on the branch (
git log --oneline upstream/develop..HEAD):b627bed8fee9359bf6e12ca275e198af6d9ce2d415122ec6d1a326903ee567c12272fdde61f7e2c11ee305ba3259e7cc53059e6f679f57d4c8f6c067d9e4852bCI checks that ran against this PR for the first time (all postdate the previous head of 2026-07-21) — every one of them passed in run 33893136431 on
b627bed8:sys.modulescore-stub leak guard innodes:testa438c23c(#1642) 07-22 /39bc5090(#1687) 07-31RR_REQUIRE_DB_TESTS=1in the Build job8db7a42d(#1713) 08-03node nodes/scripts/gen-credentials.mjs --checkinside Shell API contract210c9c7f(#1880) 08-19ai:testconfig-key catalogue sweep (packages/ai/tests/ai/common/test_config_key_warnings.py)75a33fe6(#2166) 09-02Known external dependency: open PR #2139 (docs revamp) removes
docs/README-nodes.mdand adds adocs-schemasgate. If it merges first, the catalog row here goes away with the file and the node README may need re-shaping to the new schema; nothing else in this PR depends on it.Linked Issue
Closes #1643
History: this is the chunker portion of the closed PR #499, resubmitted on a clean branch after #499 was closed for scope (three nodes plus unrelated churn).
rerank_cohereandsearch_hybridare separate PRs.🤖 AI-assisted: prepared with Claude Code; all changes reviewed by the author.