feat(mcp): scope a session to one corpus, and make usage readable - #175
feat(mcp): scope a session to one corpus, and make usage readable#175fmasi wants to merge 1 commit into
Conversation
Four changes so an agent researching one corpus is not shown another by accident. Not access control — asking for two corpora deliberately still works; being handed the wrong one silently must not. grep was the live exposure and nothing had closed it. It walks files, not an index, and every corpus here shares a root: the two profiles both resolve to the same directory and differ only by selection rules. So an unscoped grep read every corpus at once, however well the vector collections and attachment stores were separated. It now walks exactly the files its collection's profile selects — the same set indexing used, resolved in ~0.2s — and the two profiles select disjoint sets. Verified: the same term returns work folders under the work collection and personal ones under personal. Refusing matters as much as scoping. A named collection with no profile behind it raises rather than falling back to the whole root, because that fallback would quietly do the thing the caller asked to avoid. Ambiguity now refuses too. resolve_collection used to fall back to whichever onboarding manifest was written last, which is fine with one corpus and dangerous with several. With more than one it raises and names them; with exactly one it still resolves silently. Every response echoes the collection that answered it, so cross-corpus bleed is visible in the output rather than inferred from an assumed default. Same principle as `complete` on a scan and an absent-not-empty attachment list. Filename search is restored deliberately. It worked only as a side effect of the mbox envelope bug leaving raw MIME headers in body text; fixing the parse removed it, and the usage log shows callers relying on it. grep now matches decoded attachment names, without the base64 noise. Also adds `mailrag usage`: tool shares, which arguments callers actually pass, slow calls, and scans that ran out of budget. The log was only worth writing if it gets read, and it only gets read if reading is one command rather than a fresh script. Its first week already corrected an impression — grep felt heavily used at 9% of calls against search's 53%; what made it memorable was being decisive, not frequent. tests/conftest.py now isolates MAILRAG_PROFILE_DIR as well as the attachment store. A fallback test was passing only because two real profiles exist on this machine.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| path = collection_profiles().get(collection) | ||
| if path is None: | ||
| return None | ||
| cached = _FILES_CACHE.get(path) |
There was a problem hiding this comment.
_FILES_CACHE doesn't invalidate when a profile's content changes.
_PROFILE_CACHE uses an mtime-signature cache key, so a profile edit flushes the profile→path mapping. But _FILES_CACHE is keyed by profile path only — not mtime. So if a user edits a profile's selection_rules between calls, collection_profiles() correctly reloads (mtime changed), returns the same {collection → path}, but then _FILES_CACHE.get(path) returns the old file list.
This means a grep right after a profile edit (without clear_cache() or a process restart) would scope against the stale rule set. The docstring on clear_cache() mentions re-onboarding but not profile edits.
Simplest fix: include the profile mtime in _FILES_CACHE's key, or share the signature key from _PROFILE_CACHE:
| cached = _FILES_CACHE.get(path) | |
| path = collection_profiles().get(collection) | |
| if path is None: | |
| return None | |
| try: | |
| mtime = os.path.getmtime(path) | |
| except OSError: | |
| mtime = 0.0 | |
| cached = _FILES_CACHE.get((path, mtime)) | |
| if cached is not None: | |
| return cached |
(And update the store line to _FILES_CACHE[(path, mtime)] = files.)
Or, if the use case of editing profiles mid-session is considered out of scope, at least document that caveat on clear_cache.
| # filenames gives it back deliberately, without the base64 noise. | ||
| named = [n for n in meta["attachment_names"] if rx.search(n)] | ||
| if named: | ||
| matches = [f"[attachment] {n}" for n in named] |
There was a problem hiding this comment.
No test covers this new behavior. The PR description explicitly calls out "Filename search restored deliberately" as a conscious design decision, but there's no test asserting that grep_email matches and returns [attachment] <name> entries when the regex hits an attachment filename.
The existing test_attachment_names_surfaced_but_bytes_not_searched in test_grep_email.py tests that attachment bytes aren't searched — the opposite direction. A test like this is missing:
def test_attachment_filename_is_searched(self):
with _Corpus() as root:
rows = _matches("targets.xlsx", root=root) # or whatever filename exists in _Corpus
hit = next(r for r in rows if "[attachment]" in str(r.get("matches", [])))
self.assertIn("[attachment] targets.xlsx", hit["matches"])Per the repo's TDD rule, deliberately-restored behaviour needs coverage.
| "root": corpus, | ||
| # Which corpus answered, echoed back so a caller can see the scope it | ||
| # actually got rather than the one it assumed. | ||
| "collection": collection, |
There was a problem hiding this comment.
Inconsistency with search results: _thread_to_dict / _thread_to_full_dict only add collection when it is truthy (if collection: row["collection"] = collection), so callers can test presence-of-key. Here "collection": collection is always included even when collection=None, producing {"collection": null, "scoped": false, ...}.
A caller that does if "collection" in result to detect scoped responses would get false positives from unscoped grep calls. Suggest matching the search convention:
| "collection": collection, | |
| "stop_reason": stop_reason, | |
| "elapsed_s": round(time.monotonic() - started, 3), | |
| "root": corpus, | |
| "scoped": scoped_files is not None, |
and emitting "collection" only when non-None (alongside the existing comment), or document the difference explicitly.
Review summaryThe scoping logic is well-reasoned and the refusal-over-silent-fallback design is the right call. Three findings below, two inline; one here. Stale module docstring ( The module-level docstring still describes the old resolution order:
The actual order after this PR is: explicit arg → env → single profile (auto-resolved) or raise on ambiguity → manifest as last resort. The docstring should be updated to match, since it's what a caller reads before touching any of the code. Inline findings
What's not flagged
|
1 participant
2/3 of a stack. Base: the store-isolation PR.
Isolating the attachment stores left the biggest hole open.
grep_emailwalks files rather than an index, and every corpus here shares a root — the profiles resolve to the same directory and differ only by selection rules. So an unscoped grep read every corpus at once, however well the vector collections and stores were separated. It is also the tool most likely to be reached for when someone wants "find this anywhere".Scoping. grep now walks exactly the files its collection's profile selects — the same set indexing used, resolved in ~0.2s, and the two profiles select disjoint sets. Verified: the same term returns one corpus's folders under one collection and the other's under the other. A named collection with no profile behind it raises rather than falling back to the whole root, since that fallback would quietly do the thing the caller asked to avoid.
Ambiguity refuses.
resolve_collectionused to fall back to whichever onboarding manifest was written last — fine with one corpus, dangerous with nine. With more than one it now raises and names them; with exactly one it still resolves silently.Every response echoes its collection, so cross-corpus bleed is visible in the output rather than inferred from an assumed default.
Filename search restored deliberately. It worked only as a side effect of the mbox envelope bug leaving raw MIME headers in body text (fixed in #168), and the usage log shows callers relying on it. grep now matches decoded attachment names.
mailrag usagereports tool shares, which arguments callers actually pass, slow calls, and scans that ran out of budget. The log is only worth writing if it gets read, and it only gets read if reading is one command. Its first week already corrected an impression: grep felt heavily used at 9% of calls against search's 53% — what made it memorable was being decisive, not frequent.Also records the collection→profile mapping in the manifest instead of inferring it by scanning a directory for
*.profile.json, now that scoping permanently depends on it.