feat(collections): Phase 1 docs collections (create/index/link/grants, wires loaders + gitignore walker) - #195
Conversation
…t off) The allowed-roots list gates the whole collections feature: a collection source_path must resolve inside one of these directories or create/index are refused. Default is empty (feature off) so nothing changes for existing installs. TAOSMD_COLLECTIONS_ALLOWED_ROOTS overrides the config file, matching the other server settings.
First-class collection rows in collections.db with the lifecycle
created|indexing|ready|error|archived, plus three side tables:
typed project links {taos|git, id} (metadata only, no transitivity),
grants (canonical_id, scope='collection', collection_id) UNIQUE
together, and per-file content hashes for incremental re-index.
source_path is validated against collections.allowed_roots via
resolve_within at create time. Delete is an archive alias: reversible
status change, nothing destroyed. Collection ids (col- + 12 hex) match
the agent-name grammar so content rows can live under the existing
per-agent scoping.
…arch scoping collect_files walks a collection source with simplified stdlib gitignore rules (nested files, negation, dir-only, anchored patterns), skips VCS/dependency/hidden dirs, binary files (extension + null-byte sniff), oversized files, symlink escapes (resolve_within), and anything no registered loader explicitly claims - wiring the previously-unwired loader registry into a real ingest path. ingest_folder chunks each doc (zero-dep paragraph packer, upgrade-path comment for structure-aware chunking) and routes chunks through api.ingest_batch under the collection id as agent namespace with per-chunk content-hash ids, so re-index dedups unchanged files for free. Changed and deleted files get their old rows soft-superseded (valid_to + collection-reindex marker), never deleted; the archive keeps every version. Status runs created -> indexing -> ready|error with stats and last_indexed on the row; allowed_roots is re-checked at every index. api.search gains collections/collections_only: granted collections join search_agents (grants enforced per requesting agent, archived or ungranted collections contribute nothing), and collection hits carry collection_id/file_path/source metadata.
…x, async 202 poll)
Service layer gains local-only collections_* wrappers (like the shelf
admin wrappers: the server that owns the indexed filesystem runs the
ops). HTTP adds the full contract from the design spec:
- POST /collections and POST /collections/{id}/index are admin-token
gated (fail closed), joining the shelves routes in _is_admin_route
- DELETE /collections/{id} (admin) archives reversibly; do_DELETE is new
- data plane: GET /collections[?project=], GET /collections/{id},
POST link/unlink (typed {taos|git, id}), POST grants,
DELETE /collections/{id}/grants/{agent}
- indexing is async: 202 + {status: indexing, job}, poll the GET until
ready|error; the walk runs via a new _ServiceLoop.spawn so all store
access stays on the single service-loop thread and HTTP never blocks
- GET/POST /search gain collection/collections/collections_only params
with per-agent grant enforcement
Endpoint docstring table and serve() startup summary updated.
CLI: taosmd collections list|create|index|link|unlink|grant|revoke in the shelves/tasks subcommand style; index runs synchronously and prints the final stats (the async 202 path is HTTP-only). MCP: memory_list_collections tool and a collection parameter on memory_search (grant-gated, same as the HTTP surface). api._format_hit now unwraps to the innermost user metadata so the full retrieval path exposes the same metadata contract as the BM25 path for batch-ingested rows - collection hits carry file_path/source/ collection_id on both paths.
docs/collections.md covers enabling allowed_roots, the lifecycle, query semantics with grants, the HTTP surface split (admin vs data plane), and the zero-loss guarantees. The spec gains section 9 recording the Jul 19 decisions: no tree-sitter/new deps, admin gating on create+index, ship Phase 1 alone, per-collection embedder mechanism now (global default used), taOS owns the panel, plus the typed-links/grants verdict.
… docs 20 questions with gold file paths (written before the index was built) over the repo's own docs/ folder, per section 5 of the design spec. Metric is judge-free file-level Recall@5. Two arms: semantic (real ONNX embedder; the arm the kill bar judges, run on the bench host) and a lexical fallback for hosts with no local ONNX model, which stubs the embedder so rows land and retrieves through the engine's BM25-only mode, exercising the full walk/chunk/ingest/grant/scope path. auto picks per host. Lexical smoke on this host: 47 files / 493 chunks indexed in 3.1s, Recall@5 = 19/20 = 0.950 (one miss: the LoCoMo-scorecards question, cross-file term collision on 'LoCoMo'). The question set is a tiny pre-registration file, exempted from the benchmarks/data size gitignore like the README pointer.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds filesystem-backed Collections with safe-root configuration, incremental indexing, grant-scoped search, HTTP/CLI/MCP interfaces, archival semantics, and a Recall@k evaluation benchmark with comprehensive tests. ChangesCollections indexing and storage
Search and service integration
Client interfaces
Evaluation and documentation
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant HTTPServer
participant Service
participant CollectionStore
participant Search
Operator->>HTTPServer: Create and index collection
HTTPServer->>Service: Start collection indexing
Service->>CollectionStore: Validate source and update status
Service->>CollectionStore: Store indexed file state
Operator->>HTTPServer: Search with collection scope
HTTPServer->>Search: Forward collection ids and agent
Search->>CollectionStore: Check collection grant
Search-->>HTTPServer: Return authorized collection hits
HTTPServer-->>Operator: Return search response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ts = time.time() | ||
| marker = f"collection-reindex:{ts}" | ||
| rows = vmem._conn.execute( | ||
| "SELECT id, metadata_json FROM vector_memory WHERE valid_to IS NULL" |
There was a problem hiding this comment.
WARNING: _supersede_collection_rows scans the entire vector_memory table (no agent/collection_id SQL filter) on every changed/deleted file, building metadata_json for each row and decoding it in Python.
Per re-index this is O(total_active_rows × changed_files). ingest_folder calls it once per entry in [*changed, *deleted] (collections.py:761), so a re-index of a collection with C changed files issues C full-table scans over the whole vector store. As the store grows (conversation memory from all agents lives in the same table), indexing time scales with total store size, not the collection's size. Filter the scan in SQL — at minimum restrict to rows whose agent namespace is the collection id, or add a column/index keyed on collection_id.
| skips["skipped_size"] += 1 | ||
| continue | ||
| except OSError: | ||
| skips["skipped_symlink"] += 1 |
There was a problem hiding this comment.
SUGGESTION: This except OSError catches failures from check_size (a stat/open call), not symlink escapes, yet it increments skipped_symlink. A permission error or other I/O failure on a legitimate in-root file would be silently reported as a symlink skip, masking the real cause and corrupting the skips accounting surfaced in stats.
Use a distinct skip reason (e.g. skipped_error), keeping skipped_symlink only for the resolve_within failure above.
| with open(fpath, "rb") as fh: | ||
| head = fh.read(1024) | ||
| except OSError: | ||
| skips["skipped_symlink"] += 1 |
There was a problem hiding this comment.
SUGGESTION: Same mislabel as the check_size OSError above: a failure to open() the file (permission denied, etc.) is counted as skipped_symlink. Use a distinct skip reason so the stats accurately reflect why a file was dropped.
| f"collection {collection_id!r} is archived; unarchive before indexing" | ||
| ) | ||
| store.resolve_source_path(col["source_path"]) | ||
| store.set_status(collection_id, "indexing") |
There was a problem hiding this comment.
SUGGESTION: collections_index_start validates, sets status to indexing, and returns; the actual walk is spawned separately as a background job that re-opens a fresh CollectionStore and re-validates. Between the 202 response and the background task starting, the collection is marked indexing with no in-flight guard. Two near-simultaneous POST /collections/{id}/index calls (both admin-gated) can each pass the archived/resolve checks and spawn overlapping ingest_folder jobs for the same collection, racing on ingest_batch/set_file_state/_supersede_collection_rows and producing double-supersede or duplicated stats.
Consider holding a per-collection in-memory lock, or having ingest_folder refuse to start when status is already indexing, so a second concurrent index is rejected rather than run in parallel.
Code Review SummaryStatus: No Issues Found (incremental) | Recommendation: Merge Overview
Incremental Update (since b63277e)
Files Reviewed (6 changed)
Previous Review Summaries (3 snapshots, latest commit b63277e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b63277e)Status: 5 Issues Found (1 resolved this increment; 0 new) | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Incremental Update (since 5edf175)
Files Reviewed (3 changed)
Previous review (commit 5edf175)Status: 5 Issues Found (1 resolved in this increment) | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Incremental Update (since 42ab4ad)
Files Reviewed (8 changed)
Previous review (commit 42ab4ad)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 source files + tests)
Reviewed by hy3:free · Input: 28.4K · Output: 1.6K · Cached: 126.7K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/collections.md (1)
79-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifiers to fenced code blocks.
To improve syntax highlighting and resolve markdownlint warnings (MD040), specify a language (e.g.,
textorhttp) for these fenced code blocks.📝 Proposed fix
-``` +```text GET /collections [?project=<id>] GET /collections/{id} POST /collections/{id}/link {"type": "taos"|"git", "id": "..."} POST /collections/{id}/unlink same body POST /collections/{id}/grants {"agent": "..."} DELETE /collections/{id}/grants/{agent} POST /search with collection/collections/collections_onlyAdmin (dedicated admin token, fail-closed):
-
+text
POST /collections {"name", "kind", "source_path", "embedder"?}
POST /collections/{id}/index -> 202; poll GET /collections/{id}
DELETE /collections/{id} -> archive (reversible)🤖 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 `@docs/collections.md` around lines 79 - 95, Update both fenced code blocks in the collections API documentation to include a language specifier, using text or another appropriate plain-text language, while preserving their contents and formatting.Source: Linters/SAST tools
taosmd/collections.py (1)
638-640: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNarrow the supersede query by agent and file path
_supersede_collection_rows()still scans every active row and JSON-parses them in Python for each changed/deleted file. Filter in SQL onjson_extract(metadata_json, '$.agent') = ?andjson_extract(metadata_json, '$.metadata.file_path') = ?so re-index only touches this collection’s rows.🤖 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/collections.py` around lines 638 - 640, Update _supersede_collection_rows() so its active-row SQL query filters by the collection’s agent and metadata.file_path using json_extract(metadata_json, '$.agent') and json_extract(metadata_json, '$.metadata.file_path'), binding both values as query parameters before fetchall(). Preserve the existing row processing while limiting re-indexing to this collection’s rows.tests/test_collections_ingest.py (1)
72-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: silence the unused unpacked bindings flagged by Ruff (RUF059).
skips(Lines 73, 137) andstore(Line 218) are never used in those tests. Prefix with_to keep the lint clean.Also applies to: 133-137, 216-218
🤖 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 `@tests/test_collections_ingest.py` around lines 72 - 73, Rename the unused unpacked bindings in the affected tests: change skips in test_collect_files_finds_claimed_docs and the corresponding test around lines 133–137 to _skips, and change store in the test around lines 216–218 to _store, preserving the existing unpacking behavior.Source: Linters/SAST tools
taosmd/api.py (1)
434-436: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOptional: the unwrap loop over-descends if user metadata itself contains a nested
metadatadict.The loop keeps descending while any
metadatakey is a dict, so a hit whose user metadata legitimately holds a nested dict under"metadata"would return the inner dict and drop the sibling user keys. Not reachable for collection hits (flat metadata), but worth confirming no other producer nests a user-controlledmetadatafield.🤖 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/api.py` around lines 434 - 436, Review the metadata producers feeding user_md and confirm whether nested user-controlled “metadata” dictionaries are possible. Update the unwrapping logic around user_md so it removes only the known outer metadata wrapper rather than recursively descending through legitimate nested user metadata, preserving sibling keys at the user-metadata level.
🤖 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/collections.py`:
- Around line 692-793: Update ingest_folder around CollectionStore and its
existing try/except so the store is closed on every exit path: retain the
current status/error handling, nest it under an outer try/finally, and call
store.close() in that finally block after successful completion or exceptions.
- Around line 731-737: Update the empty-text branch in the collection indexing
flow around the visible prior hash comparison so previously indexed files are
treated as changed: supersede their active rows and remove their file state
before continuing. Preserve the existing skip behavior for files that were never
indexed, while ensuring an emptied file no longer retains searchable rows or its
stored hash.
---
Nitpick comments:
In `@docs/collections.md`:
- Around line 79-95: Update both fenced code blocks in the collections API
documentation to include a language specifier, using text or another appropriate
plain-text language, while preserving their contents and formatting.
In `@taosmd/api.py`:
- Around line 434-436: Review the metadata producers feeding user_md and confirm
whether nested user-controlled “metadata” dictionaries are possible. Update the
unwrapping logic around user_md so it removes only the known outer metadata
wrapper rather than recursively descending through legitimate nested user
metadata, preserving sibling keys at the user-metadata level.
In `@taosmd/collections.py`:
- Around line 638-640: Update _supersede_collection_rows() so its active-row SQL
query filters by the collection’s agent and metadata.file_path using
json_extract(metadata_json, '$.agent') and json_extract(metadata_json,
'$.metadata.file_path'), binding both values as query parameters before
fetchall(). Preserve the existing row processing while limiting re-indexing to
this collection’s rows.
In `@tests/test_collections_ingest.py`:
- Around line 72-73: Rename the unused unpacked bindings in the affected tests:
change skips in test_collect_files_finds_claimed_docs and the corresponding test
around lines 133–137 to _skips, and change store in the test around lines
216–218 to _store, preserving the existing unpacking 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: 17f7b52a-52aa-4a6c-ae59-ed3686fccc55
📒 Files selected for processing (19)
.gitignoreCHANGELOG.mdbenchmarks/collections_eval.pybenchmarks/data/collections_eval_questions.jsondocs/collections.mddocs/specs/codebase-indexing-collections-design.mdtaosmd/api.pytaosmd/cli.pytaosmd/collections.pytaosmd/config.pytaosmd/http_server.pytaosmd/mcp_server.pytaosmd/service.pytests/test_collections_cli.pytests/test_collections_http.pytests/test_collections_ingest.pytests/test_collections_mcp.pytests/test_collections_store.pytests/test_config_collections_roots.py
…nwrap The unwrap-to-innermost loop gave every path the same user-metadata contract but stripped the provenance envelope on the way down: batch rows are double-wrapped on the semantic path (retrieval envelope -> row meta -> user metadata), so archive_span_id vanished from the formatted hit and the prefer_verified claims gate went blind for those rows (a contradicted-claim row survived recall). Capture archive_span_id/agent/project while descending and re-attach them to a copy of the innermost metadata (user keys never clobbered), so the gate keeps its inputs and collection hits still expose file_path. Regression tests cover the semantic and bm25 paths plus the contradicted-row drop.
The 202+poll index contract still blocked the single service loop: collect_files did the full os.walk, per-file size checks, and the 1KB null-byte sniff (opening every candidate file) synchronously on the loop thread, so /search and /ingest stalled for the duration of an index. Run the walk and the per-file hash+chunk step through asyncio.to_thread; the store writes stay on the loop where the thread-affine sqlite connections live. Test pins the walk to a worker thread and that the pipeline still lands created -> indexing -> ready.
collections_index_start set status=indexing without checking it, so a
second POST /collections/{id}/index while one index was running would
double-walk the same tree into the batch dedup. Raise
CollectionBusyError when the status is already indexing; the endpoint
maps it to 409 with a poll hint. ready and error both re-arm the
start, so retries after completion or failure keep working.
The walker had a per-file size cap but no tree cap, so a collection pointed at a huge tree (a home dir, a monorepo root) would grind through every file. collect_files now raises past DEFAULT_MAX_FILES (20000, upgrade-path comment for making it configurable) and the index errors cleanly with the offending path and a pointer to narrow the source folder; ingest_folder passes the cap through for callers.
Make the Jul 19 data-plane-grants decision explicit rather than a surprise: grants scope collections per agent inside the server-token boundary, any holder of the server token can manage grants, and collection access is exactly as strong as that token. Only create/index/delete sit behind the admin token because they touch the server filesystem. Also document the 20000-file walk cap and the 409 on concurrent index starts.
|
Adversarial review findings addressed. Fixes pushed as five commits, each with a regression test written first and confirmed failing before the fix. 1. Claims gate went blind for batch rows (high) - 2. Async index blocked the service loop (medium) - 4. No concurrent-index guard (low) - 5. No tree cap on the walker (low) - 3. Grants on the data plane - intended Jul 19 decision, left as is; docs/collections.md now states the trust model explicitly (grants scope collections per agent inside the server-token boundary; any holder of the server token can manage grants; collection access is as strong as the server token). Finding 6's gitignore simplification already carries its upgrade-path marker. ( Suite: 1186 passed (was 1178; +8 tests). |
|
Semantic eval result (the pre-registered kill-criterion arm), run on the Fedora bench host with the real MiniLM ONNX embedder rather than the lexical fallback: Protocol as specced: the repo's own Ship-bar status for Phase 1:
Held for review. |
…es empty A file whose content was emptied still exists on disk, so it stayed in the walker's seen set and never reached the deleted path: its old vector rows were never superseded and its hash state was never cleared, leaving stale content searchable indefinitely against the incremental/zero-loss contract in the function's own docstring. Drop such a file from seen so the existing supersede path handles it (rows stamped valid_to with the hidden_by marker, never hard-deleted) and its file state is cleared. A file that was already blank is absent from the prior state too, so it stays a no-op and does not churn on re-index.
ingest_folder opened its own CollectionStore and never closed it on any path, so a server re-indexing on a timer leaked one sqlite connection (and file handle) per run. The service wrappers all close theirs in a finally; this path was the outlier. Wrap the whole body in try/finally so the connection closes on success and on the error path that records status=error.
The two HTTP surface blocks had bare fences (markdownlint MD040).
|
Thanks for the review. Both majors were genuine bugs, not false positives. Fixed in 5a9c0b6, 5621882, b63277e. 1. Emptied file left stale rows searchable (zero-loss contract violation) Correct call. A previously-indexed file whose content became blank hit the Fix: an emptied file is now dropped from Regression tests: 2. Store connection leaked in Also correct. Fix: the whole body is wrapped in Regression tests: Nitpick
Full suite green at 1190 passed (was 1186, plus the 4 new tests). |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_collections_ingest.py (2)
335-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
storevariable (Ruff RUF059).
store, col = _make_collection(...)never usesstorein this test. Elsewhere in the same file (line 389) the unused return is correctly bound to_store; this instance is inconsistent and will keep tripping the linter.♻️ Proposed fix
- store, col = _make_collection(data_dir, source_dir) + _store, col = _make_collection(data_dir, source_dir)🤖 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 `@tests/test_collections_ingest.py` around lines 335 - 345, Update test_reindex_already_empty_file_is_a_no_op so the unused first return value from _make_collection is bound to the file’s established unused-variable placeholder, matching the nearby test pattern; keep col unchanged for the ingest calls.Source: Linters/SAST tools
368-381: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNarrow the blind
except Exception(Ruff B017).
with pytest.raises(Exception):arounds._conn.execute("SELECT 1")will also pass if execution fails for an unrelated reason (e.g._connmissing/mocked wrongly), silently hiding a broken assertion. SQLite raisessqlite3.ProgrammingErrorfor a closed connection — assert that specifically.🐛 Proposed fix
+import sqlite3 + for s in made: - with pytest.raises(Exception): + with pytest.raises(sqlite3.ProgrammingError): s._conn.execute("SELECT 1")🤖 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 `@tests/test_collections_ingest.py` around lines 368 - 381, Update the closed-connection assertion in test_ingest_folder_closes_the_store_on_success to expect sqlite3.ProgrammingError instead of the broad Exception type when executing s._conn.execute("SELECT 1"). Ensure the test imports sqlite3 as needed and preserves the existing validation that every tracked store is closed.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@tests/test_collections_ingest.py`:
- Around line 335-345: Update test_reindex_already_empty_file_is_a_no_op so the
unused first return value from _make_collection is bound to the file’s
established unused-variable placeholder, matching the nearby test pattern; keep
col unchanged for the ingest calls.
- Around line 368-381: Update the closed-connection assertion in
test_ingest_folder_closes_the_store_on_success to expect
sqlite3.ProgrammingError instead of the broad Exception type when executing
s._conn.execute("SELECT 1"). Ensure the test imports sqlite3 as needed and
preserves the existing validation that every tracked store is closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba9681be-f5a4-4a88-864f-6144c4ce0e94
📒 Files selected for processing (3)
docs/collections.mdtaosmd/collections.pytests/test_collections_ingest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/collections.md
- taosmd/collections.py
A previously-indexed file whose content becomes empty was routed through the deleted path and counted in files_deleted, conflating two things a UI needs to show apart: a file that vanished from disk versus a file that still exists but has no content left. Split the reporting with a distinct files_emptied counter. The underlying behaviour is unchanged: both cases still supersede their old rows and clear their file_states hash, so the zero-loss retirement holds. The key is always present (0 when none) so the stats shape stays stable.
1 participant
What
Phase 1 of collections (the docs MVP from
docs/specs/codebase-indexing-collections-design.md): named containers of content indexed from a folder, queryable by granted agents alongside conversation memory. This wires the previously-unwired loader framework (taosmd/loaders/) into a real ingest path and adds the full container lifecycle, access control, and query surface. HELD FOR REVIEW: do not merge without sign-off.Why
Codebase indexing is table stakes for coding agents; docs collections ship first (per the Jul 19 decision) to get real usage on the container model before the Phase 2 code path. Everything is additive and off by default: with no
collections.allowed_rootsconfigured, nothing changes for existing installs.Binding decisions implemented (Jul 19)
# upgrade-path:comments.POST /collectionsandPOST /collections/{id}/index;DELETE /collections/{id}(archive) is admin too. Data plane (list/get/link/unlink/grants/query) uses the normal token + grants.embedderfield: stored at create, returned in GET, read by the index path; Phase 1 always indexes with the global default.(canonical_id, scope='collection', collection_id)UNIQUE together, stored in taosmd, enforced on search per requesting agent; no transitivity (project links never grant access).{type: taos|git, id}, multiple per collection, link/unlink metadata-only.valid_tomachinery.collections.allowed_rootsconfig list (default EMPTY = feature off),resolve_withincontainment at create and at every index, symlink escapes rejected,check_sizeper file.How it works
taosmd/collections.py: sqlite store (collections.db) for rows/links/grants + per-file content hashes; gitignore-aware walker (simplified stdlib fnmatch rules, VCS/dependency/hidden dirs, binary sniff, size cap);ingest_folderchunks and routes throughapi.ingest_batchunder the collection id as agent namespace, so per-chunk content-hash ids dedup unchanged files and the archive keeps every version.search_agentsmechanism:api.searchgainscollections/collections_onlyand extends the agent set with granted, non-archived collections only.{"status": "indexing", "job": id}, pollGET /collections/{id}untilready|error; the walk runs via a new_ServiceLoop.spawnso store access stays on the single service-loop thread and HTTP never blocks.Endpoints
Plus CLI
taosmd collections list|create|index|link|unlink|grant|revokeand MCPmemory_list_collections+collectiononmemory_search.Tests
test_admin_surfacerandom-order flake did not trip.Eval (pre-registered, section 5 of the spec)
benchmarks/collections_eval.py+ a 20-question gold-file set over the repo's owndocs/(tracked inbenchmarks/data/collections_eval_questions.json, written before the index was built). No local ONNX model exists on this dev host, so the smoke ran the lexical (BM25) fallback arm, which stubs the embedder only so rows land and retrieves through the engine'smode="bm25"path (full walk/chunk/ingest/grant/scope path exercised, vectors never consulted):--mode semanticon the Fedora bench host.Spec deviations (flagged)
source_pathis required on create (spec body marked it optional). The docs MVP has nothing to do with a source-less collection; making it required keepscreatefrom minting rowsindexmust then reject.project_idbody field; project attachment is exclusively via the typedlinkendpoint (one mechanism instead of two, matches the typed-links decision).collection_files(per-file content hashes), backs incremental re-index; storing the hash map instatsJSON would have made stats writes racy with indexing.created|indexing|ready|error|archived(decisions list) rather than the spec section-2empty|...wording.RemoteClientforwarding), matching the shelf admin wrappers: the server that owns the indexed filesystem runs the ops. Remote-configured CLI installs would need a follow-up to proxy them.api._format_hitnow unwraps to the innermost user metadata so the semantic path exposes the same metadata contract as the BM25 path for batch-ingested rows (previously the full path surfaced the row envelope). No test relied on the old double-nested shape.Docs:
docs/collections.mduser page, endpoint docstring table + serve() summary, CHANGELOG entry, spec section 9 (Decisions 2026-07-19).Summary by CodeRabbit
embedderfield (Phase 1 uses the global default).