fix(corpus): enforce max_documents cumulatively across .jsonl files in directory walks#230
Conversation
…es (OpenAgentHQ#224) In `_load_documents`' directory branch the cumulative cap check (`len(documents) >= self.max_documents`) sat only inside the non-jsonl `else` branch, so it never ran after a `.jsonl` file. `_load_jsonl_documents` caps within a single file only (its local list resets per call), so a directory of `.jsonl` files silently blew past `max_documents` — a memory / safety bound — while a directory of `.txt` files capped correctly. Move the cap check out of the `else` so it runs after both branches, and trim the extended list to `max_documents` before breaking. Behavior for single-file loads and `.txt`-only directories is unchanged (a per-file `.txt` append never overshoots, so the trim is a no-op there). Regression: a directory of 2 jsonl files x 3 lines with max_documents=2 now loads exactly 2 (was 4), and a mixed jsonl+txt directory caps at 2 (was 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/oc review pr |
Review:
|
|
🎉 Congratulations @Nitjsefnie! Your pull request has been successfully merged into main. 🚀 Thank you for contributing to OpenAgentHQ and helping improve the project. We truly appreciate your contribution and hope to see you back with more amazing PRs! Happy Open Sourcing! ❤️ |
Use slicing (documents[:self.max_documents]) instead of in-place delete (del documents[self.max_documents:]) to match the fix already merged in PR #230.
2 participants
Fixes #224
The cumulative
max_documentscheck in_load_documents' directory branch sat inside the non-jsonlelse, so it never ran after a.jsonlfile was processed — the cap was enforced per-file, never cumulatively (grows unbounded with more jsonl files). The check now runs after both branches, trimmingdocumentsto the cap before breaking. Single-file loads and non-jsonl directories are behaviorally unchanged (the per-file append never overshoots there, so the trim is a no-op) — all of #222's merged regression tests stay green.New tests: 2 jsonl files × 3 lines with
max_documents=2→ exactly 2 (fails on main withassert 4 == 2, matching the issue repro), and a mixed jsonl+txt directory capping correctly (fails on main withassert 3 == 2). Full suite: 961 passed, 4 skipped. The two ruff findings inauditor.pyare pre-existing on main (verified) — new code adds none.Generated by Claude Fable 5 (brief, review), Claude Opus 4.8 (implementation)