Internal bookkeeping paths are never corpus content - #183
Merged
Conversation
Five witnesses against ingestion, all failing. The Files API already draws
this line and says so in its own comment: any relative path with a component
beginning "." is bookkeeping, omitted from listings and treated as absent by
download and delete. Ingestion never learned it.
The default extension list includes ".json", so a directory source rooted at
a user's files/ walks into .checksums.json and chunks it. Measured, the
indexed text is:
{" ferrothorn. txt":{" checksum":" 527c75bd8631…"," contexts":[]}}
which is the user's own filenames and checksums, retrievable as though
somebody had uploaded it.
Cases are about components rather than basenames, because the rule is:
bundle/.internal/secret.md is internal for the same reason the manifest is,
and a fix matching the manifest by name would leave it indexed.
Two of the five exist to stop the fix landing in the wrong place. One drives
the Files API and the walker over one tree and requires that what the listing
omits is exactly what ingestion refuses, rather than reading the two
implementations and believing they agree. The other names .checksums.json as
a source directly: authorize_path grants authority over anything under the
caller's own users/{id} directory and says nothing about bookkeeping, so that
path reaches the single-file branch, which never walks a directory at all.
The invariant is "never corpus", not "directory walks skip it".
`is_internal_path` moves to service/fs.py, beside the other path-policy rules, and every surface asks the same one. The Files API spelled it twice — inline in the listing and again as `_is_hidden_relpath` — and corpus ingestion did not ask at all, which is how the upload manifest became a document. The rule is about components, not basenames. Matching `.checksums.json` by name would fix the sighting and leave `bundle/.internal/secret.md` indexed, so the shared predicate keeps the Files API's own definition: any component beginning "." is bookkeeping. Both of `ingest_path`'s branches ask it. The walk asks of each entry relative to the source root; the single-file branch asks of the name it was handed, because `authorize_path` grants authority over anything under the caller's own directory and says nothing about which files in it are content. A source rooted at a hidden directory is refused whole, since its children look ordinary relative to it. Authority and classification stay separate. A caller is entitled to read their own manifest and it is still not a document. Four mutations, each applied and each killed: removing the walker's refusal, weakening the rule to the basename, narrowing it to the manifest's exact name, and dropping the single-file check. The file budget does not depend on where the check sits. `files_processed` is incremented only after a successful ingest, so a path that continues before that leaves the budget untouched wherever the test appears; what keeps a tree full of bookkeeping from starving real documents is that the entries are refused at all. The witness for it says so, and passes against the unfixed code whenever the walk happens to yield a document first.
Review found two more callers of the internal-path rule, both the same shape as the defect this tranche started from: the predicate was correct and the caller asked it at the wrong altitude, or never asked. The source was classified by its basename. `bundle/.internal/secret.md` has an ordinary basename and an internal position, so naming it outright as a context source indexed it, while the identical file reached by walking `files/` was refused. One file, two answers. A source rooted at `.internal/subdir` was the same story from the other side: its children look ordinary relative to it, so nothing in the walk objected. `is_internal_under(base, path)` states the two errors it sits between. The absolute path must not be scanned, because whether a deployment lives under `/srv/.storage` is its own spelling and would refuse the whole installation; and the basename alone is not enough, for the reason above. Both production callers already pass a base, so the strong form is what runs. Classifying the source once, before the file/directory branch, let two basename checks collapse into one. The durable queue never asked at all. Re-indexing calls `rag.ingest_file` directly, so every refusal added to the walk was invisible to it — and it is the machinery a replacement actually runs through, so an internal path reaching `ingest_job` would be chunked on a schedule long after whoever created it stopped watching. That also contradicted the SPEC line this tranche adds: refused by any route. The job is closed `superseded` with the reason recorded rather than failed, because nothing is owed now or later and a failure would be retried five times to reach the same conclusion. One of the new witnesses was vacuous as first written. The queue case enqueued a placeholder generation, so the job was declined as stale before ingestion was attempted and "no chunks written" passed for an unrelated reason. The detail column said so, which is what it was added for. It now enqueues the generation the bytes actually have. Six mutations, all applied and all killed. The two new ones land where they should: classifying the source by basename kills only the direct-source cases, and removing the queue's check kills only the queue case.
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
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.
The upload manifest was a document. Measured, the indexed chunk reads
— a user's own filenames and checksums, sitting in the corpus where a retrieval can answer out of it. Nobody uploaded that.
The rule already existed, in one place
The Files API draws this line and says so in its own comment: hidden components are internal bookkeeping, uploads and extraction strip leading dots, so a user can never own such a name. Listings omit them; download and delete treat them as absent.
Ingestion never learned it.
.jsonis in the default extension list, so a directory source rooted atfiles/— the obvious source to add, being everything the user has uploaded — walked straight into the manifest.Where the rule lived is worth noting: spelled twice inside
routes.pyalone, once inline in the listing and once as_is_hidden_relpath, and a third time nowhere. A predicate with two copies and one missing caller is the shape this defect is made of.Components, not basenames — and position, not spelling
The obvious patch is
if file_path.name == ".checksums.json": continue, which fixes the sighting rather than the class.bundle/.internal/secret.mdis internal for the same reason and would stay indexed.Two predicates, because two different questions are being asked:
is_internal_path(relative)— the component rule, over a path already expressed relative to a namespace.is_internal_under(base, path)— the same question asked of an absolute path, by first putting it in the frame that gives "internal" a meaning. It sits between two opposite errors: the absolute path must not be scanned, because whether a deployment lives under/srv/.storageis its own spelling and would refuse an entire installation; and the basename alone is not enough, becausebundle/.internal/secret.mdhas an ordinary basename and an internal position.Every ingestion route, not only the walk
path.nameonlyfs_root, job closedsuperseded_is_hidden_relpathThe queue was the seam that mattered most. Re-indexing calls
rag.ingest_filedirectly, so every refusal added to the walk was invisible to it — and the queue is the durable machinery a replacement actually runs through, so an internal path reachingingest_jobwould be chunked on a schedule long after whoever created it stopped watching. The job is closed rather than failed: nothing is owed now or later, and a failure would be retried five times to reach the same conclusion.Authorization and classification stay separate, which is why the direct-source case exists at all.
authorize_pathgrants authority over anything under the caller's ownusers/{id}directory and says nothing about bookkeeping. A caller is entitled to read their own manifest, and it is still not a document.What the file budget actually depends on
A tree full of bookkeeping must not exhaust
max_filesbefore reaching anything a user wrote. It does not — but the reason is narrower than where the check sits.files_processedis incremented only after a successful ingest, so a path thatcontinues before that leaves the budget untouched wherever the test for it appears. What makes the property hold is that internal entries are refused at all.That distinction is recorded because it was nearly mis-stated as a claim about ordering, and a code comment asserting it had already been written before the claim was checked. The witness for the budget says plainly that it pins a property rather than reproducing a failure — it passes against the unfixed code whenever the walk happens to yield a real document first.
Verification
Nine witnesses, six failing before the fix. Two exist to hold the fix at the right altitude: one drives the Files API and the corpus walker over one tree and requires that what the listing omits is exactly what ingestion refuses; one drives
POST /v1/contexts/{id}/sources, parameterized over a file named beneath.internaland a directory rooted there.Six mutations, all applied, all killed, and the two narrowest land exactly where they should:
Lanes:
make test-xdist— 2884 passed, 27 skipped, 0 failed. CI's lint selection clean.Two corrections made during the work
A mutation reported
SURVIVEDwhile measuring nothing: its replacement ended in an escaped quote inside a raw string, producing an unterminated literal, so pytest never ran. The driver now treats a run with no summary line as a broken build and reads stderr as well as stdout.The queue witness was vacuous as first written. It enqueued a placeholder generation, so the job was declined as stale before ingestion was attempted, and "no chunks were written" passed for a reason unrelated to the path — the
detailcolumn saidon-disk generation 2faf4dced1fb, which is what that column was added for. It now enqueues the generation the bytes actually have, and the unfixed queue does index the internal file.🤖 Generated with Claude Code
https://claude.ai/code/session_01DQtPsg9YSUXaStGXyUjozA
Generated by Claude Code
Note
Overview
Fixes a disclosure where
.checksums.json(and other dot-component paths) could be indexed as RAG corpus—e.g. after ingesting a user's entirefiles/tree—even though the Files API already hides them from listings and downloads.Centralizes the rule in
service.fsasis_internal_path(any path component starting with.) andis_internal_under(same check for absolute paths relative to an authorized base). The Files API listing and_is_hidden_relpathnow call the shared predicate instead of duplicating inline logic.Corpus ingestion applies it everywhere: directory walks skip internal entries;
ingest_pathclassifies named sources against their authorized base (not basename-only); the durable ingest queue closes internal jobs assupersededinstead of callingingest_filedirectly. SPEC now states internal paths are never content and must be refused by any route.Adds
tests/test_internal_paths_are_not_corpus.pyto lock listing/walker agreement, nested hidden dirs, direct sources, and queue behavior.Reviewed by Cursor Bugbot for commit 86bb25c. Bugbot is set up for automated code reviews on this repo. Configure here.