feat: add moss-chunking - pluggable chunking strategies over a shared chunk contract - #505
Open
adityachawla005 wants to merge 14 commits into
Open
feat: add moss-chunking - pluggable chunking strategies over a shared chunk contract#505adityachawla005 wants to merge 14 commits into
adityachawla005 wants to merge 14 commits into
Conversation
… chunk contract
Chunking isn't in any Moss SDK, so every app rolls its own. The two Python ones
in this repo both already wrap chunks in DocumentInfo and still can't be treated
uniformly: moss-pikachu emits {path}#chunk-0001 with path/filename/chunk/
extension/modified_at, moss-llamaindex emits {filename}-p{page}-c{idx} with
source/page. Zero metadata keys in common.
Splitting strategy is contested and content-dependent, so it stays pluggable via
a ChunkingStrategy Protocol. The output isn't contested, so this pins it: stable
zero-padded IDs, and a declared locator unit (char/line/page) rather than a fixed
position field list, since offsets are meaningless for a PDF and pages are
meaningless for a source file.
Ships CharSplitter (pikachu's 1800/300), SentenceSplitter (llamaindex's 400
words / 2 sentences, regex-based so there's no nltk corpus to provision),
ParagraphSplitter and RecursiveSplitter — the sentence/paragraph/recursive set
named in ROADMAP.md:112, with semantic deferred since embeddings are a different
shape of dependency. Layout mirrors moss-data-connector; ingest drops the
template's auto_id, since random UUIDs would defeat the stable IDs.
Python only for now — vscode and moss-md-indexer are TS.
Depends only on moss. 48 tests.
adityachawla005
requested review from
HarshaNalluru,
ashvathsureshkumar and
r4ghu
as code owners
August 4, 2026 14:56
|
|
|
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:
📝 WalkthroughWalkthroughAdded the Changesmoss-chunking package
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant chunk_document
participant ChunkingStrategy
participant Chunk
participant DocumentInfo
Caller->>chunk_document: provide text, source, and strategy
chunk_document->>ChunkingStrategy: split(text)
ChunkingStrategy-->>chunk_document: ordered Chunk values
chunk_document->>Chunk: to_document(source)
Chunk-->>DocumentInfo: return text, ID, and metadata
chunk_document-->>Caller: return DocumentInfo list
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codex reviewNo issues found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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 @.github/workflows/publish-moss-chunking.yml:
- Around line 116-120: The publish job rebuilds the distribution instead of
reusing the artifact validated in build-test, so untested bytes can be uploaded
to PyPI. In build-test (around line 66-70, after the "Build distributions"
step), add an upload-artifact step (e.g. actions/upload-artifact) that runs only
on one designated matrix leg (e.g. python == '3.11') and uploads the
packages/moss-chunking/dist/ directory as a named artifact, using
if-no-files-found: error; keep other matrix legs as import-only smoke tests. In
the publish job, remove the "Build distributions" step (lines 116-120) and
replace it with an actions/download-artifact step that downloads the same named
artifact into packages/moss-chunking/dist, so publish uploads the exact artifact
that was smoke-tested. Adjust publish job dependencies (e.g. remove reliance on
the build tool if no longer needed) so only twine is required there.
- Around line 84-86: Update the smoke test’s moss_chunking import check to
iterate over module.__all__ and assert that every declared public name resolves
on the imported module, replacing the non-assertive dir(module)[:5] output while
preserving the import validation.
- Line 107: Replace the mutable version tags in the publish job's actions with
full 40-character commit SHAs to prevent supply-chain attacks through
compromised tags. Specifically, update the actions/checkout@v4 and
actions/setup-python@v5 references to pin to their respective commit SHAs, and
include the original version number in a trailing comment for maintainability
and future reference.
- Around line 29-40: Replace the regex-based version parsing with Python 3.11's
standard library tomllib to properly parse the pyproject.toml file. Update the
import statement to include tomllib instead of re, then modify the version
extraction logic to parse the TOML content using tomllib and access the version
from the project table (parsed_data["project"]["version"]) rather than using
regex matching. Remove the unused re module from the imports, and ensure the
parsed version value is written to GITHUB_OUTPUT as before.
- Around line 99-105: Update the release flow around the duplicate-release guard
and twine upload so reruns tolerate an already-uploaded PyPI version and can
still create and push the missing tag. Make the tag check validate an exact tag
reference rather than any object resolved by git rev-parse, using the existing
moss-chunking version tag name, and preserve failure for genuinely conflicting
releases.
- Line 19: Update the checkout steps in the determine-version and build-test
jobs to set persist-credentials to false. Leave the publish job checkout
unchanged so its credentials remain available for pushing the release tag.
- Around line 92-93: Update the publish job in the workflow to use the pypi
environment with explicit approval and grant only the OIDC token-generation
permission required for trusted publishing. Replace the credential-based PyPI
upload and PYPI_API_TOKEN secret usage with the trusted-publishing action, while
preserving the existing package build and release flow.
In `@packages/moss-chunking/pyproject.toml`:
- Around line 14-19: In the classifiers section of
packages/moss-chunking/pyproject.toml, add a new classifier entry for Python
3.14 after the existing "Programming Language :: Python :: 3.13" line to match
the format and pattern of the other Python version classifiers. This aligns the
declared classifier support with the requires-python setting and the tested
versions in the publish workflow.
- Around line 54-55: Update the pytest.ini_options configuration near
asyncio_mode to set asyncio_default_fixture_loop_scope explicitly to the stable
scope required by the async ingest tests, preserving the existing automatic
asyncio mode.
In `@packages/moss-chunking/README.md`:
- Around line 96-102: Update the custom-strategy example in the README to import
Iterator from collections.abc and Chunk from moss_chunking before defining
MyStrategy. Align the documented return annotation with the ChunkingStrategy
protocol by using Iterator[Chunk] consistently in the surrounding prose or
snippet.
In `@packages/moss-chunking/src/chunk.py`:
- Around line 59-64: The extra field on Chunk is a mutable dict, which allows
callers to bypass the reserved-key validation done in __post_init__ by mutating
extra after construction, and also breaks hashability of the frozen Chunk
dataclass. Change the extra field's stored representation to an immutable,
hashable structure such as a tuple of (key, value) tuples, updating the
default_factory accordingly, and adjust __post_init__ validation and to_document
(which currently splats **self.extra) to build a dict from this immutable
structure when constructing metadata. Ensure chunk_document's use of
dataclasses.replace continues to work with the new representation and that
reserved-key validation still runs on every construction.
- Around line 79-81: Update the chunk dataclass’s __post_init__ validation near
the RESERVED_KEYS check to inspect every self.extra value and reject any
non-string value before to_document runs, raising an error that identifies the
offending key. Add a test covering a non-string extra value and asserting it is
rejected.
- Around line 38-48: The 4-digit chunk index formatting breaks lexicographic
ordering at index 10000. In packages/moss-chunking/src/chunk.py lines 38-48,
widen the padding or enforce the supported index bound in chunk_id; in
packages/moss-chunking/tests/test_chunk.py lines 9-13, include indices through
10000 to verify ordering; in packages/moss-chunking/README.md lines 60-63,
document the index limit required by the sorting guarantee.
In `@packages/moss-chunking/src/strategies.py`:
- Line 219: The DEFAULT_SEPARATORS tuple contains `". "` which causes trailing
periods to be excluded from chunks because `_split_spans` sets piece ends to
match.start(), excluding the matched separator entirely. To preserve the period
in chunk text while keeping separators as literals, modify `_split_spans` to
include the matched text with the preceding piece for terminator-bearing
separators like `". "`. Add test coverage that exercises the `". "` separator
path to verify chunks retain their sentence terminators and maintain consistency
with how SentenceSplitter handles terminators.
- Around line 132-133: Remove the undocumented overlap clamp from the
initialization of overlap_sentences so the caller’s explicit value is preserved;
progress is already guaranteed by the cursor update in the chunking loop. Update
test_overlap_is_clamped_so_progress_is_guaranteed to verify that group_start
advances and splitting terminates, rather than asserting a clamped overlap
value.
In `@packages/moss-chunking/tests/test_chunk.py`:
- Around line 40-44: Rename test_extra_metadata_is_merged_and_stringified to
reflect that it only verifies metadata merging, then add a separate test
covering a non-string extra value and asserting that Chunk processing and
to_document produce the expected stringified metadata. Use the existing Chunk
and to_document APIs and preserve the current merge assertions.
In `@packages/moss-chunking/tests/test_strategies.py`:
- Around line 36-44: Update roundtrips and every call site, including
test_offsets_slice_back_to_the_chunk_text and the other uses later in the file,
to assert the roundtrip condition separately for each chunk rather than
collapsing results with all(...). Preserve the existing offset slice comparison
so pytest identifies the offending chunk and its failing offsets.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8ef9e31e-6274-4768-b71f-716303f96699
⛔ Files ignored due to path filters (1)
packages/moss-chunking/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/publish-moss-chunking.ymlpackages/moss-chunking/.gitignorepackages/moss-chunking/README.mdpackages/moss-chunking/pyproject.tomlpackages/moss-chunking/src/__init__.pypackages/moss-chunking/src/chunk.pypackages/moss-chunking/src/enrich.pypackages/moss-chunking/src/ingest.pypackages/moss-chunking/src/strategies.pypackages/moss-chunking/tests/test_chunk.pypackages/moss-chunking/tests/test_strategies.py
…econstruction RecursiveSplitter dropped the non-whitespace part of any separator it cut on. Splitting "abc. def." at max_chars=4 returned "abc" and "def.", losing the first period: _split_spans discards whatever the boundary matches, which is right for "\n\n" or " " but wrong for ". ", where the period ends the sentence and only the space separates. A separator's non-whitespace head now goes into a lookbehind so the cut lands after the punctuation instead of through it. The roundtrip invariant did not catch this. Offsets stayed self-consistent — they just stopped covering the text between them. Added a test asserting the gap between adjacent chunks is only ever whitespace, which is the property that was actually missing. frozen=True freezes Chunk.extra the field, not the dict behind it, so a reserved key could be written in after __post_init__ had already validated it, and to_document would render it over the contract's own metadata. extra is now copied on construction, and merged first at render so the five reserved keys win regardless. prepend_context rewrote doc.text while carrying the old embedding forward, pairing a vector with content it no longer describes and quietly skewing the dense half of hybrid queries. It now drops the embedding, forcing a recompute downstream — the alternative is a rule that enrichment must precede embedding, which nothing can enforce. The same reconstruction was also dropping payload, which unlike the embedding has nothing to do with the text — silent data loss rather than a decision. It is now carried through. 57 tests.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ity, floor
Non-string extra values (3 reviewers). to_document promised all-string metadata
but passed extra through unchanged, so extra={"page": 3} failed at the SDK
boundary instead of rendering "3". Coercion is back and the field is typed
dict[str, object], since {"page": 3} is the natural thing to write.
Chunk index bound (3 reviewers). :04d stops being fixed width at 10000, so
chunk-10000 sorted before chunk-9999 and the ID format's only promise broke
silently. chunk_id now rejects an index above MAX_CHUNK_INDEX rather than
emitting an unsortable ID; four digits stays for pikachu parity.
Chunk was not hashable. frozen=True has the dataclass advertise hashability, but
the dict field made hash(chunk) raise TypeError, so chunks could not go in a set
or key a dict. extra is excluded from the hash and still compared for equality.
CRLF paragraph boundaries. _PARAGRAPH_BOUNDARY matched only LF, so a
Windows-authored file had no paragraph breaks at all and ParagraphSplitter
emitted one chunk far past max_chars.
Dependency floor. moss>=1.1.1 cannot resolve: every release before 1.7.2 pins an
inferedge-moss-core that is no longer on PyPI. 1.7.2 is the earliest installable
version, and the earliest whose DocumentInfo is verified to accept payload —
which is what the reported TypeError was really about.
Release gate. The matrix only proved the wheel imported; it now runs the suite
against the installed wheel and asserts every __all__ name resolves, so a build
that corrupts offsets or metadata cannot reach PyPI. Version parsing uses
tomllib instead of a regex, the tag guard matches refs/tags exactly, and twine
gets --skip-existing so a partial upload can be rerun.
The SentenceSplitter overlap clamp is kept for llamaindex parity, but its comment
claimed it was what stopped a large overlap stalling progress. It is not — the
group_start + 1 floor in split() is, for any overlap. Comment corrected and a
test pins the real guarantee.
Also: 3.14 classifier to match requires-python and the test matrix, README
imports for the custom-strategy example, and per-chunk roundtrip assertions so a
failure names the offending chunk.
64 tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/publish-moss-chunking.yml:
- Around line 142-144: Update the publish workflow’s twine upload step so
existing PyPI distributions are verified against the locally built files, or
cause the job to fail before the tag step instead of accepting an all-skipped
upload. Also pin the twine version installed by the workflow to ensure
consistent upload behavior.
In `@packages/moss-chunking/src/chunk.py`:
- Around line 53-54: Update the source validation in the chunking entry points,
including chunk_id and to_document, to reject any value that is not a string
before formatting IDs or emitting metadata; retain the existing non-empty
requirement for strings. Add a test covering a truthy non-string source such as
an integer.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 39b933bc-e1b7-435a-8359-6d65037856f8
⛔ Files ignored due to path filters (1)
packages/moss-chunking/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/publish-moss-chunking.ymlpackages/moss-chunking/README.mdpackages/moss-chunking/pyproject.tomlpackages/moss-chunking/src/__init__.pypackages/moss-chunking/src/chunk.pypackages/moss-chunking/src/strategies.pypackages/moss-chunking/tests/test_chunk.pypackages/moss-chunking/tests/test_strategies.py
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ecedence CharSplitter could emit the same span twice. Windows advance along the raw text but chunks are the trimmed span inside them, so on padded input two consecutive windows trim to the same piece: chunk_chars=10, overlap=5 over " abcde fghij" yielded "abcde" at (5,10) as both chunk 0 and chunk 1 — identical content indexed twice under two IDs. Emission now skips any span that ends at or before the last one, so every chunk adds new content. extra is now a read-only view on a copy, not just a copy. The previous fix left it mutable while it still counted towards equality, so a chunk already in a set could change what it equals while its hash stayed put and the set could no longer find its own member. Immutability removes the divergence rather than papering over it, and answers the reviewers who asked for an immutable mapping. Source-level extra no longer overwrites a chunk's own metadata. chunk_document merged the document-wide dict last, so a splitter that knows the page or section number had it silently replaced by a constant. The chunk's value wins now. ingest promised something it did not do: it always calls create_index, so the stable-ID re-indexing it described could never happen through it. The docstring now says it is the create path and points at MossClient.add_docs, which is where stable IDs actually replace rather than append. chunk_id rejects a non-string source. chunk_id(123, 0) formatted happily and put a non-string into metadata, which is the boundary failure this module exists to catch. Reverted twine --skip-existing. Added last round at one reviewer's suggestion, another then pointed out it lets the tag step run against artifacts PyPI already has from a different commit. Plain upload is the template's behaviour and fails loudly instead. 69 tests.
adityachawla005
commented
Aug 5, 2026
adityachawla005
commented
Aug 5, 2026
adityachawla005
commented
Aug 5, 2026
adityachawla005
commented
Aug 5, 2026
MAX_CHUNK_INDEX was enforced only in chunk_id, which runs at to_document time, so a chunk past the bound was accepted at construction and failed later — mid-iteration inside chunk_document, far from where the index was set. The same check now runs in __post_init__, so the two validation sites agree. 70 tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/publish-moss-chunking.yml:
- Line 142: Make the publish workflow’s upload step recoverable after a
successful PyPI upload but failed release-tag push. Before treating existing
artifacts as complete, verify that every published file’s PyPI digest matches
the locally built distribution, then allow the workflow to continue to tag
creation; reject mismatches or missing files and retain normal upload behavior
for unpublished files. Do not reintroduce unverified --skip-existing behavior,
and anchor the change around the twine upload step and subsequent release-tag
creation/push commands.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea7bda5d-6b54-49d8-bba9-d58ee3b72fb0
📒 Files selected for processing (6)
.github/workflows/publish-moss-chunking.ymlpackages/moss-chunking/src/chunk.pypackages/moss-chunking/src/ingest.pypackages/moss-chunking/src/strategies.pypackages/moss-chunking/tests/test_chunk.pypackages/moss-chunking/tests/test_strategies.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/moss-chunking/src/chunk.py`:
- Around line 95-102: Update the index validation in both the Chunk constructor
and chunk_id function to require an integer while explicitly rejecting bool
before performing range checks. Preserve the existing bounds validation and
error behavior for valid integers, and add regression tests covering fractional
and boolean indices in both entry points.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a83cc60d-86ee-4a89-a7c8-16fa6df4c145
📒 Files selected for processing (2)
packages/moss-chunking/src/chunk.pypackages/moss-chunking/tests/test_chunk.py
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
bool subclasses int, so Chunk(text, True, ...) formatted as a perfectly valid chunk-0001 and nothing complained. A float was accepted at construction and then died at the :04d format with "Unknown format code 'd'", a long way from whatever set the index. Both checks now live in one _require_index helper called from chunk_id and __post_init__, which also removes the bound check that was duplicated across the two after the last fix. 75 tests.
The publish job holds PYPI_API_TOKEN and pushes a release tag, and workflow_dispatch accepts any ref — so an unmerged branch could be published under the package name and tagged as a release. Guarded on refs/heads/main. None of the repo's 20 publish workflows has this guard; not fixing the others here, but a new one should not ship without it. MappingProxyType was the wrong way to make extra immutable. It is not picklable, which took deepcopy and dataclasses.asdict down with it, and it still could not stop a mutable *value* — a list held in extra — from changing a chunk's equality after it went into a set. Both come from treating metadata as part of identity. A chunk is its text and its position; what is recorded about it does not change which cut of the document it is. extra is now a plain dict copy excluded from eq and hash, so the obvious dataclass paths work again and nothing extra holds can make a chunk in a set unfindable, by rebinding or by mutation. 75 tests.
adityachawla005
commented
Aug 5, 2026
moss 1.7.2's runtime DocumentInfo does accept payload — its signature is (id, text, metadata=None, embedding=None, payload=None) and the value round trips — but the SDK's shipped __init__.pyi stub declares only the first four. Code that passes the kwarg unconditionally is therefore correct at runtime and wrong to a type checker reading the stub. Passing it conditionally keeps the package right against both, and degrades to dropping payload only on a build that has no such field to lose. The test skips rather than fails in that case, so the package is not pinned to one runtime shape of DocumentInfo. 75 tests.
adityachawla005
commented
Aug 5, 2026
…dices Reject non-string keys in `Chunk.extra` at construction rather than coercing them: `str(1)` and "1" are the same metadata key, so coercion would let one entry overwrite another, and an uncoerced key slips past the reserved-key check to fail at the SDK boundary. Validate in `chunk_document` that a strategy numbers its chunks 0, 1, 2, …. A repeat renders the same ID twice and the second chunk overwrites the first on ingest — silent data loss. Renumbering would hide that just as quietly and rewrite the addressing of a splitter that meant something by its index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adityachawla005
commented
Aug 5, 2026
adityachawla005
commented
Aug 5, 2026
…ail behind `add_docs` upserts but does not reconcile: a document re-cut from 21 chunks into 6 leaves #chunk-0006..0020 in the index, still searchable, holding text the document no longer contains. Nothing errors and the stale hits look real. `refresh_source` deletes that tail before the new chunks go in. Finding it is cheap because the contract guarantees contiguous indices, so leftovers sit in one run above the new chunk count and a single window of candidate IDs past the end either finds it or proves it absent. Passing no documents removes the source entirely, which is how a deleted file leaves the index. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adityachawla005
commented
Aug 5, 2026
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ugin Delete the stale tail highest ID first. Lowest-first punched a hole under the survivors of a failed batch: the next probe read the emptied low window as proof nothing was left and stopped, stranding every ID above it in the index with no run that would ever reach them. Deleting a suffix instead leaves a contiguous prefix, which is exactly what the next refresh walks. Wait on every deletion job. `delete_docs` returns when the job is accepted, not when it has run, so an unwaited deletion that failed afterwards was invisible — refresh_source returned a successful add while the stale chunks stayed searchable. Failures now raise rather than being reported as a refresh. Sort probe results before deleting, since `get_docs` promises no order. Install pytest-asyncio in the publish workflow's test job: the suite's async tests rely on asyncio_mode = "auto", and plain pytest fails them outright. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adityachawla005
commented
Aug 5, 2026
adityachawla005
commented
Aug 5, 2026
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The fake client in these tests cannot catch a method the real one lacks, and `wait_for_job` reads as missing from both the checked-in stub and the in-tree SDK source (1.0.0b19, old enough to predate it). It is present on the declared floor, moss==1.7.2, which is the surface that decides — so assert against the installed client rather than leaving the question to whoever reads the stub. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
refresh_source reads len(documents) as the first index the source no longer uses. That is only true when the documents are that source's entire cut, so another source's documents — or a filtered slice of this one's — would delete live chunks and then add documents belonging to something else. IDs are now checked against chunk_id(source, position) before anything is deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adityachawla005
commented
Aug 5, 2026
adityachawla005
commented
Aug 5, 2026
…ency note Locators get the same treatment as the index: a float or bool passed the range checks and rendered as "1.5" or "True" in metadata meant to hold a real offset. `_require_whole_number` is now shared by both. Re-check extra's keys in to_document. `extra` stays a mutable dict — that is what keeps a chunk picklable and copyable — so a key added after construction reached DocumentInfo unvalidated. The check now runs where metadata is built, not only where it was set. Document that refresh_source is not atomic. Concurrent refreshes of one source can interleave; the index is the shared resource and no client-side lock makes probe/delete/add one operation. The delete ordering does bound the damage to ordinary stale tail, which the next refresh clears. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adityachawla005
commented
Aug 5, 2026
adityachawla005
commented
Aug 5, 2026
adityachawla005
commented
Aug 5, 2026
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
2 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.
Pull Request Checklist
Description
Chunking isn't in any Moss SDK today, so every app that needs it writes its own. The two Python ones in this repo are a good example — they both already wrap their chunks in
DocumentInfo, so the container is shared, but underneath they agree on almost nothing:moss-pikachumoss-llamaindex{path}#chunk-0001{filename}-p{page}-c{idx}path,filename,chunk,extension,modified_atsource,pageNot a single metadata key in common. So anything downstream that wants to treat chunks uniformly - dedupe them, re-ingest them, point back to their source - can't, because there's no shared shape to rely on.
This package fixes that without freezing the part that should stay flexible. As came up in the contributors channel, how you split text is genuinely contested - code, prose, and transcripts all want different strategies - so that stays pluggable. What isn't contested is what a chunk should look like once it's out, so that's what this pins down.
The contract (
chunk.py) - every chunk, whatever produced it, comes out with:{source}#chunk-{index:04d}, zero-padded so IDs sort in cut order and stay identical across runs (re-chunking an unchanged doc reproduces them, so chunks replace instead of piling up as duplicates)char,line, orpage— rather than a fixed set of fields, because a character offset means nothing for a PDF and a page number means nothing for a source fileDocumentInfooutput, rendered once by the contract (strategies never build it themselves), with every value stringified since Moss types metadata asDict[str, str]The strategies (
strategies.py) — a smallChunkingStrategyProtocol (split(text) -> Iterable[Chunk]) and four implementations:CharSplitterandSentenceSplittermatch pikachu and llamaindex exactly (the two duplications this replaces), andRecursiveSplitterandParagraphSplitterround out the set named in the roadmap. Semantic splitting is deliberately left out for now — it needs embedding calls during ingestion, which is a different shape of dependency, and the README says so.There's also an optional
enrichstep that carries over pikachu's trick of prepending the filename and path into the chunk text so BM25 can match on them — kept as a separate compose-able step, so pikachu can adopt it without having to switch splitters.Addresses ROADMAP.md:112 (built-in text splitters).
Scope: this is the package half of what was discussed. The in-SDK zero-config default splitter is a natural follow-up against
sdks/python/— happy to do that next if the interface here looks right. Python only for now;vscodeandmoss-md-indexerare TypeScript and out of scope for this.Notes for reviewers
ingest.pymirrors the connector template but drops itsauto_idoption on purpose — random UUIDs would defeat the stable IDs above.publish-moss-chunking.ymlis modelled on the zeroentropy connector (feat(connectors): add moss-connector-zeroentropy #490), which shipped its publish workflow in the same PR.Type of Change
48 tests passing, ruff clean under CI's pinned 0.15.22.
Summary by CodeRabbit
New Features
Documentation
Chores