feat(loaders): typed ingestor package (cognee-style LoaderInterface) - #80
Merged
Conversation
Closes task #56. Lifts cognee's LoaderInterface ABC (66 lines at cognee/infrastructure/loaders/LoaderInterface.py) with memobase's discriminated-union envelope (BlobType: chat | transcript | email | doc) on top. taosmd/loaders/ __init__.py — package exports interface.py — LoaderInterface ABC (loader_name, supported_extensions, supported_mime_types, can_handle, async load -> Blob) blob.py — typed envelopes: Blob (base), ChatBlob, TranscriptBlob, EmailBlob, DocBlob. Stdlib dataclasses only — no pydantic dependency to keep Pi-tier dep matrix tight. chat_loader.py — JSON list / {messages: [...]} -> ChatBlob. Multi-suffix '.chat.json' / '.messages.json' extension matching. transcript_loader.py — Whisper segments / canonical TranscriptStamp list / plain list -> TranscriptBlob. JSON only (VTT/SRT deferred). email_loader.py — RFC 5322 .eml via stdlib email -> EmailBlob. Threading headers (Message-Id, In-Reply-To) preserved for downstream thread-tree work. doc_loader.py — text / markdown -> DocBlob. Markdown title extracted from first '# ' heading. registry.py — pick_loader(path) -> LoaderInterface. First registered loader whose can_handle() returns True wins; DocLoader catches all. Scope-honest note: this PR ships the abstraction. process_conversation_turn and the existing string-based ingest paths keep working unchanged — nothing migrated to the new path. Migration is a separate refactor (swap call sites once we have a real consumer driving the choice). Test plan - 20 new tests in tests/test_loaders.py exercising each loader against its expected file format, the registry's path-extension extraction (multi-suffix support), pick_loader for each blob type + the unknown- extension fallback to DocLoader, register_loader's insert-before-doc ordering, and an end-to-end pick+load. - All 192 total tests pass (172 before + 20 new).
📝 WalkthroughWalkthroughThis PR introduces a complete typed loader framework to ChangesLoader Framework
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
taosmd/loaders/chat_loader.py (1)
49-50: ⚡ Quick winSpecify
encoding="utf-8"when reading JSON.
open(path)uses the platform default encoding (e.g. cp1252 on Windows), which can corrupt non-ASCII content even though JSON is defined as UTF-8.DocLoaderalready pinsencoding="utf-8"; align here (and intranscript_loader.pyline 60).♻️ Proposed fix
- with open(path) as f: + with open(path, encoding="utf-8") as f: data = json.load(f)🤖 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 `@taosmd/loaders/chat_loader.py` around lines 49 - 50, The JSON file is opened with the platform default encoding; change the open call in chat_loader.py (the block using with open(path) as f: data = json.load(f)) to explicitly use utf-8 (open(path, encoding="utf-8")) so non-ASCII content isn't corrupted, and make the same change in transcript_loader.py at the analogous open(...) call around line 60 to match DocLoader's encoding behavior.
🤖 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 `@taosmd/loaders/chat_loader.py`:
- Around line 51-70: The code currently treats data["messages"] as iterable
without verifying its type, causing TypeError for non-iterables and silent
issues for strings; in ChatLoader when handling the branch that sets
raw_messages from data["messages"], add an explicit isinstance(..., list) check
(similar to TranscriptLoader) and raise a ValueError if it's not a list, so
iterate only over a validated list and preserve the existing handling of the
top-level list branch and the construction of ChatMessage objects.
In `@taosmd/loaders/email_loader.py`:
- Around line 32-35: The load method currently uses
email.message_from_binary_file which treats an entire .mbox as a single message;
change load in EmailLoader (method load(file_path: str | Path) that returns
EmailBlob) to detect .mbox files and, when path.suffix == ".mbox", open the mbox
via mailbox.mbox(str(path)) and extract only the first message (e.g.,
next(iter(box), None)) before converting to the EmailBlob fields, handling an
empty mailbox gracefully; keep the existing email.message_from_binary_file
behavior for non-mbox files so single-message files still work.
In `@taosmd/loaders/registry.py`:
- Around line 55-86: The _path_to_extension function fails to return the
multi-suffix form for canonical files like "transcript.json" or "chat.json",
causing pick_loader to misroute to DocLoader; update _path_to_extension so that
when the final suffix is "json" and the immediate preceding name is "transcript"
or "chat" it returns "transcript.json"/"chat.json" (e.g. detect parts[-1] ==
"json" and parts[-2] in {"transcript","chat"} and return ".".join(parts[-2:]));
otherwise keep the existing fallback (single-suffix behavior). This fixes
LoaderInterface.can_handle matching and lets TranscriptLoader/ChatLoader be
selected by pick_loader.
---
Nitpick comments:
In `@taosmd/loaders/chat_loader.py`:
- Around line 49-50: The JSON file is opened with the platform default encoding;
change the open call in chat_loader.py (the block using with open(path) as f:
data = json.load(f)) to explicitly use utf-8 (open(path, encoding="utf-8")) so
non-ASCII content isn't corrupted, and make the same change in
transcript_loader.py at the analogous open(...) call around line 60 to match
DocLoader's encoding behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a291153-1a77-4781-8f2e-890dee36dfa2
📒 Files selected for processing (10)
taosmd/__init__.pytaosmd/loaders/__init__.pytaosmd/loaders/blob.pytaosmd/loaders/chat_loader.pytaosmd/loaders/doc_loader.pytaosmd/loaders/email_loader.pytaosmd/loaders/interface.pytaosmd/loaders/registry.pytaosmd/loaders/transcript_loader.pytests/test_loaders.py
Comment on lines
+51
to
+70
| if isinstance(data, dict) and "messages" in data: | ||
| raw_messages = data["messages"] | ||
| elif isinstance(data, list): | ||
| raw_messages = data | ||
| else: | ||
| raise ValueError( | ||
| f"ChatLoader: {path} doesn't match chat shape " | ||
| "(expected list of messages or {messages: [...]})" | ||
| ) | ||
|
|
||
| messages: list[ChatMessage] = [] | ||
| for m in raw_messages: | ||
| if not isinstance(m, dict): | ||
| continue | ||
| messages.append(ChatMessage( | ||
| role=str(m.get("role", "")), | ||
| content=str(m.get("content", "")), | ||
| alias=str(m.get("alias", "")), | ||
| timestamp=float(m.get("timestamp", 0.0) or 0.0), | ||
| )) |
There was a problem hiding this comment.
Validate that messages is a list before iterating.
Unlike TranscriptLoader (which guards isinstance(data["transcripts"], list)), here a {"messages": ...} object whose value is non-iterable (e.g. an int) raises a TypeError at for m in raw_messages, and a string value silently yields zero messages. Tighten the shape check.
🐛 Proposed fix
- if isinstance(data, dict) and "messages" in data:
- raw_messages = data["messages"]
+ if isinstance(data, dict) and isinstance(data.get("messages"), list):
+ raw_messages = data["messages"]
elif isinstance(data, list):
raw_messages = data📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if isinstance(data, dict) and "messages" in data: | |
| raw_messages = data["messages"] | |
| elif isinstance(data, list): | |
| raw_messages = data | |
| else: | |
| raise ValueError( | |
| f"ChatLoader: {path} doesn't match chat shape " | |
| "(expected list of messages or {messages: [...]})" | |
| ) | |
| messages: list[ChatMessage] = [] | |
| for m in raw_messages: | |
| if not isinstance(m, dict): | |
| continue | |
| messages.append(ChatMessage( | |
| role=str(m.get("role", "")), | |
| content=str(m.get("content", "")), | |
| alias=str(m.get("alias", "")), | |
| timestamp=float(m.get("timestamp", 0.0) or 0.0), | |
| )) | |
| if isinstance(data, dict) and isinstance(data.get("messages"), list): | |
| raw_messages = data["messages"] | |
| elif isinstance(data, list): | |
| raw_messages = data | |
| else: | |
| raise ValueError( | |
| f"ChatLoader: {path} doesn't match chat shape " | |
| "(expected list of messages or {messages: [...]})" | |
| ) | |
| messages: list[ChatMessage] = [] | |
| for m in raw_messages: | |
| if not isinstance(m, dict): | |
| continue | |
| messages.append(ChatMessage( | |
| role=str(m.get("role", "")), | |
| content=str(m.get("content", "")), | |
| alias=str(m.get("alias", "")), | |
| timestamp=float(m.get("timestamp", 0.0) or 0.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 `@taosmd/loaders/chat_loader.py` around lines 51 - 70, The code currently
treats data["messages"] as iterable without verifying its type, causing
TypeError for non-iterables and silent issues for strings; in ChatLoader when
handling the branch that sets raw_messages from data["messages"], add an
explicit isinstance(..., list) check (similar to TranscriptLoader) and raise a
ValueError if it's not a list, so iterate only over a validated list and
preserve the existing handling of the top-level list branch and the construction
of ChatMessage objects.
Comment on lines
+32
to
+35
| async def load(self, file_path: str | Path, **kwargs) -> EmailBlob: | ||
| path = Path(file_path) | ||
| with open(path, "rb") as f: | ||
| msg = email.message_from_binary_file(f, policy=policy.default) |
There was a problem hiding this comment.
.mbox handling parses the whole file as a single message, not the first one.
email.message_from_binary_file reads the entire stream as one RFC 5322 message. For a multi-message mbox, headers come from the first message but the body absorbs every subsequent message (separated by From lines), contradicting the docstring's "first message in the file is used." Single-message mbox files are fine; multi-message ones produce a polluted EmailBlob.body.
Since multi-message support is explicitly deferred, at minimum extract just the first message via the mailbox module to keep the documented contract honest.
🐛 Sketch — read only the first mbox message
import mailbox
# for .mbox:
box = mailbox.mbox(str(path), factory=None)
msg = next(iter(box), None) # first message, or handle empty🤖 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 `@taosmd/loaders/email_loader.py` around lines 32 - 35, The load method
currently uses email.message_from_binary_file which treats an entire .mbox as a
single message; change load in EmailLoader (method load(file_path: str | Path)
that returns EmailBlob) to detect .mbox files and, when path.suffix == ".mbox",
open the mbox via mailbox.mbox(str(path)) and extract only the first message
(e.g., next(iter(box), None)) before converting to the EmailBlob fields,
handling an empty mailbox gracefully; keep the existing
email.message_from_binary_file behavior for non-mbox files so single-message
files still work.
Comment on lines
+55
to
+86
| def _path_to_extension(path: str | Path) -> str: | ||
| """Return the lowercased extension, including multi-suffix forms. | ||
|
|
||
| ``meeting.transcript.json`` → ``transcript.json`` (not just ``json``) | ||
| so loaders that claim a multi-suffix shape get a chance. | ||
| """ | ||
| p = Path(path) | ||
| name = p.name.lower() | ||
| if "." not in name: | ||
| return "" | ||
| # Try two-suffix form first ("transcript.json"), then one-suffix. | ||
| parts = name.split(".") | ||
| if len(parts) >= 3: | ||
| return ".".join(parts[-2:]) | ||
| return parts[-1] | ||
|
|
||
|
|
||
| def pick_loader(file_path: str | Path) -> LoaderInterface: | ||
| """Return an instance of the first registered loader that claims | ||
| the given path. Falls back to ``DocLoader`` (the catch-all). | ||
| """ | ||
| p = Path(file_path) | ||
| ext = _path_to_extension(p) | ||
| mime_type, _ = mimetypes.guess_type(str(p)) | ||
| mime_type = mime_type or "" | ||
|
|
||
| for loader_cls in REGISTRY: | ||
| if loader_cls.can_handle(extension=ext, mime_type=mime_type): | ||
| return loader_cls() | ||
| # Belt-and-braces — REGISTRY ends with DocLoader so this is | ||
| # unreachable, but defend against someone removing the catch-all. | ||
| return DocLoader() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'interface.py' | xargs rg -nP -C4 'def can_handle'Repository: jaylfc/taosmd
Length of output: 570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Print full LoaderInterface.can_handle implementation (including matching semantics).
# First locate interface.py path(s).
paths="$(fd -t f 'interface.py' .)"
echo "$paths" | head -n 20
# Pick the first match (most likely singular); if multiple, the grep below will show all.
for f in $paths; do
echo "---- $f: LoaderInterface.can_handle context ----"
# Show from def can_handle through the next few defs/classes.
rg -n "def can_handle" "$f" -n || true
# Print ~40 lines around can_handle
rg -n "def can_handle" "$f" -n --context 0 || true
# Use sed to grab the region around the match(s)
# (handle multiple matches by printing the first occurrence only)
line="$(rg -n "def can_handle" "$f" | head -n1 | cut -d: -f1)"
if [ -n "${line:-}" ]; then
start=$((line-5))
end=$((line+80))
sed -n "${start},${end}p" "$f" | nl -ba | sed 's/^/ /'
fi
done
# 2) Find TranscriptLoader.can_handle and any multi-suffix/transcript-related loader.
# Search for "TranscriptLoader" or "transcript" across loaders.
echo "---- Transcript-related loader can_handle overrides ----"
rg -n "class\s+\w*Transcript\w*Loader|\bTranscriptLoader\b" taosmd/loaders -S || true
rg -n "can_handle\(" taosmd/loaders -S --glob '*transcript*' || true
rg -n "transcript\.json|transcript json|transcript" taosmd/loaders -S --glob '*py' || true
# 3) Also locate chat-related loader if present.
echo "---- Chat-related loader can_handle overrides ----"
rg -n "class\s+\w*Chat\w*Loader|\bChatLoader\b" taosmd/loaders -S || true
rg -n "can_handle\(" taosmd/loaders -S --glob '*chat*' || true
rg -n "chat\.json|chat json|chat" taosmd/loaders -S --glob '*py' || true
# 4) Inspect registry.py for REGISTRY ordering.
echo "---- registry.py REGISTRY ----"
sed -n '1,220p' taosmd/loaders/registry.py | nl -ba | sed 's/^/ /'Repository: jaylfc/taosmd
Length of output: 381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print LoaderInterface.can_handle body (no nl dependency)
f="./taosmd/loaders/interface.py"
echo "---- interface.py can_handle implementation ----"
line="$(rg -n "def can_handle" "$f" | head -n1 | cut -d: -f1)"
echo "can_handle at line: $line"
start=$((line-10))
end=$((line+120))
sed -n "${start},${end}p" "$f" | cat -n
echo
echo "---- Transcript-related loaders (classes + can_handle overrides) ----"
rg -n "class\s+\w*Transcript\w*Loader|\bTranscriptLoader\b" taosmd/loaders -S --glob '*py' || true
rg -n "can_handle\(" taosmd/loaders -S --glob '*transcript*' || true
rg -n "transcript\.json|transcript json|Transcript" taosmd/loaders -S --glob '*py' || true
echo
echo "---- Chat-related loaders (classes + can_handle overrides) ----"
rg -n "class\s+\w*Chat\w*Loader|\bChatLoader\b" taosmd/loaders -S --glob '*py' || true
rg -n "can_handle\(" taosmd/loaders -S --glob '*chat*' || true
rg -n "chat\.json|chat json|Chat" taosmd/loaders -S --glob '*py' || true
echo
echo "---- registry.py REGISTRY ordering ----"
sed -n '1,220p' taosmd/loaders/registry.py | cat -nRepository: jaylfc/taosmd
Length of output: 11748
Fix loader routing for canonical transcript.json / chat.json filenames
_path_to_extension("transcript.json") / _path_to_extension("chat.json") returns "json". LoaderInterface.can_handle matches supported_extensions using exact normalized equality (case-insensitive, leading dot optional), and TranscriptLoader/ChatLoader further rely on extension ending with "transcript.json" / "chat.json". With extension="json", these loaders don’t claim the file, so pick_loader falls through to DocLoader instead of returning the typed TranscriptBlob/ChatBlob.
🐛 Proposed fix — also try the full two-suffix form
p = Path(file_path)
ext = _path_to_extension(p)
mime_type, _ = mimetypes.guess_type(str(p))
mime_type = mime_type or ""
+ # Short names like "chat.json" yield ext="json"; also offer the
+ # full two-suffix form so multi-suffix loaders can still claim them.
+ name_parts = p.name.lower().split(".")
+ multi_ext = ".".join(name_parts[-2:]) if len(name_parts) >= 2 else ext
for loader_cls in REGISTRY:
- if loader_cls.can_handle(extension=ext, mime_type=mime_type):
+ if loader_cls.can_handle(extension=ext, mime_type=mime_type) or \
+ loader_cls.can_handle(extension=multi_ext, mime_type=mime_type):
return loader_cls()🤖 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 `@taosmd/loaders/registry.py` around lines 55 - 86, The _path_to_extension
function fails to return the multi-suffix form for canonical files like
"transcript.json" or "chat.json", causing pick_loader to misroute to DocLoader;
update _path_to_extension so that when the final suffix is "json" and the
immediate preceding name is "transcript" or "chat" it returns
"transcript.json"/"chat.json" (e.g. detect parts[-1] == "json" and parts[-2] in
{"transcript","chat"} and return ".".join(parts[-2:])); otherwise keep the
existing fallback (single-suffix behavior). This fixes
LoaderInterface.can_handle matching and lets TranscriptLoader/ChatLoader be
selected by pick_loader.
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
1 participant
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
Closes task #56 — the last pending architectural task from the May 10 survey of memory-system competitor patterns. Lifts cognee's `LoaderInterface` ABC + memobase's discriminated-union envelope into taosmd as a typed-ingestor package.
What lands
`taosmd/loaders/` — 9 modules, 936 lines total:
What this changes (and doesn't)
Does: adds an alternative typed ingest path for callers with format-specific data on disk — meetings as Whisper transcripts, exported chat sessions, individual emails, plain markdown notes. Each comes back as a typed envelope so downstream extractors can lean on structured fields (speaker, timestamp, in_reply_to) instead of parsing a flattened string.
Doesn't: migrate any existing call site. `process_conversation_turn` and the string-based ingest paths keep working unchanged. Migration is a separate, smaller refactor once we have a real consumer driving the choice (an agent that wants to ingest a real Whisper transcript and exploit the per-speaker timestamps).
Test plan
20 new tests covering every loader + the registry + an end-to-end pick+load:
All 192 total tests pass (172 before + 20 new). No external deps added.
Open follow-ups (NOT in this PR)
Summary by CodeRabbit
New Features
Tests