Refs #157 (slice 4). risk/high: control code, adversarial review required.
Problem
Each of these read notes by path, so a symlinked note (Syncthing syncs symlinks) put the target's title, aliases and wikilinks into the place listed, and a FIFO hung the read:
graphsync.IndexDoc / BackfillFromBrainDir → the knowledge graph (entities and edges)
wiki.LoadInventory → the wiki inventory, and the LLM prompts the pipeline builds from it
wiki.RebuildIndex → page summaries in wiki/index.md
Change
graphsync: reads go through brainfs. The backfill walks with WalkRegular, skips a missing root, and aborts on any other walk error, including a symlinked root.
wiki: the new brainfs.ReadDirRegular requires every component of the dir to be a real directory, and lists regular files only. Frontmatter and first sentences are read through the root. A symlinked type dir is an error.
brainfstest: the outside file carries the sentinel in the frontmatter title and aliases, a heading and a wikilink, so extractors that ignore the body are covered too.
Tests
All red before the fix: 186k–378k leaks in the races, and the static symlink cases failed. All have positive controls. The new concepts tests initially used concept, and their positive controls caught that.
Flips (red)
IndexDoc reverted to os.ReadFile
backfill ignores walk errors
ReadDirRegular lists non-regular entries
ReadDirRegular without realDir
Not flipped: inventory frontmatter through os.ReadFile. The patch doesn't compile without a signature change.
Not covered
RebuildIndexwriteswiki/index.md with os.WriteFile, which follows a symlink. That's a write path, tracked in #158.
Refs #157 (slice 4). risk/high: control code, adversarial review required.
## Problem
Each of these read notes by path, so a symlinked note (Syncthing syncs symlinks) put the target's title, aliases and wikilinks into the place listed, and a FIFO hung the read:
- `graphsync.IndexDoc` / `BackfillFromBrainDir` → **the knowledge graph** (entities and edges)
- `wiki.LoadInventory` → **the wiki inventory**, and the LLM prompts the pipeline builds from it
- `wiki.RebuildIndex` → page summaries in **`wiki/index.md`**
## Change
- **graphsync:** reads go through brainfs. The backfill walks with `WalkRegular`, skips a missing root, and aborts on any other walk error, including a symlinked root.
- **wiki:** the new `brainfs.ReadDirRegular` requires every component of the dir to be a real directory, and lists regular files only. Frontmatter and first sentences are read through the root. A symlinked type dir is an error.
- **brainfstest:** the outside file carries the sentinel in the frontmatter title and aliases, a heading and a wikilink, so extractors that ignore the body are covered too.
## Tests
All red before the fix: 186k–378k leaks in the races, and the static symlink cases failed. All have positive controls. The new `concepts` tests initially used `concept`, and their positive controls caught that.
## Flips (red)
- `IndexDoc` reverted to `os.ReadFile`
- backfill ignores walk errors
- `ReadDirRegular` lists non-regular entries
- `ReadDirRegular` without `realDir`
**Not flipped:** inventory frontmatter through `os.ReadFile`. The patch doesn't compile without a signature change.
## Not covered
- `RebuildIndex` **writes** `wiki/index.md` with `os.WriteFile`, which follows a symlink. That's a write path, tracked in #158.
- Remaining readers: brain index/tunnel/linkcheck, frontmatter scan, pipeline, provenance, classification.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---
_Created via git-mcp on behalf of @mathias_
graphsync.IndexDoc/BackfillFromBrainDir and wiki.LoadInventory/
RebuildIndex read notes by path (os.ReadFile, os.Open, os.ReadDir +
open). A symlinked note put the target's title, aliases and wikilinks
into the knowledge graph, the wiki inventory (and the LLM prompts built
from it) and wiki/index.md; a FIFO hung each read.
- graphsync: IndexDoc and the backfill read through brainfs; the
backfill walks with WalkRegular, skips a missing root and aborts on
any other walk error, including a symlinked root.
- wiki: LoadInventory lists type dirs with the new
brainfs.ReadDirRegular (every component a real dir, regular files
only) and reads frontmatter through the root; a symlinked type dir is
an error. RebuildIndex reads first sentences through the root.
- brainfstest's outside file now carries the sentinel in frontmatter
title and aliases, a heading and a wikilink, so extractors that ignore
the body are tested too.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pr-gatekeeper verdict: BLOCK (PR #168 @ c7a6429). Not merged.
CI run 9176 is green. Locally, CI=1 task check exits 0 with 0 FAILs, and -race -count=5 on graphsync, wiki, brainfs and pipeline is green. None of that covers the findings below. All of them were found by running code: probes and flips in a scratch copy of the module. The worktree was not modified.
Blocking
1. Every new symlink test points at a file outside the brain, and os.Root refuses those anyway. So the Lstat layer this PR depends on has no test in either package. This is the same class as the earlier rounds.
Each flip below was run against ./internal/graphsync/ ./internal/wiki/ ./internal/brainfs/.
Flip
Result
indexDoc reads through a no-Lstat reader (readOpened)
green
readFrontmatter reads through a no-Lstat reader
green
pageFirstSentence reads through a no-Lstat reader
green
ReadDirRegular Lstats only the last component (wiki/ itself not checked)
green in all three packages. There is no brainfs unit test for ReadDirRegular at all.
ReadRegular drops its Lstat type check
graphsync and wiki green; only brainfs.TestReadRegularRefusesSymlinksAndReadsRegularFiles goes red
These flips are not harmless. Under the indexDoc bypass, a static in-brain link wiki/w/facts/inlink.md -> ../../../raw/secret.md puts raw/'s title and wikilinks into the graph (IndexDoc err=nil, 2 sentinel hits). With the PR's code it is refused. Fix: add an in-brain-target case next to each outside-target case (a file link into raw/, and wiki -> raw/w for ReadDirRegular), plus a brainfs unit test for ReadDirRegular.
2. Rule 9: TestRebuildIndexDoesNotReadAPageSwappedForASymlink and the two ...SurvivesSwapRaces tests claim something the code doesn't do. I ran the same swap harness (brainfstest.StartSwap) at HEAD, unmodified, with the link pointing inside the brain (../../raw/s.md) instead of outside it, 1.5 s each:
Reader
Leaks of raw/ content
RebuildIndex → wiki/index.md
8,229 in 55k iterations
LoadInventory → inventory (LLM prompts)
9,528 in 93k
IndexDoc → graph
109,472 in 232k
Cause:ReadRegular Lstats the path and then opens it again by path. If the entry is swapped for an in-root symlink in between, os.Root follows it, and the handle check passes because the target is a regular file. This is a file swap inside ReadRegular. #167 accepts a directory swap during a walk, which is a different gap. The reasoning #167 gives (in-brain only, a microsecond window, the tripwire) probably applies here too, but nobody has decided that.
Fix, either:
(a) Compare the Lstat result with the handle's stat using os.SameFile in ReadRegular/ReadRegularMax. I tested a sketch: all three races went to 0 leaks (242k, 102k and 73k iterations). That fix also covers slices 1–3, which share ReadRegular.
(b) Mathias explicitly extends #167 to cover file swaps, and the tests are renamed to say outside the brain (for example ...DoesNotReadAnOutsideFileSwappedIn).
As the names stand, they are false.
3. Rule 9: two tests say "Skips" when the code aborts.
TestLoadInventorySkipsASymlinkedTypeDir: the code returns an error and the whole inventory is lost (probe: err=... walk root component wiki/concepts is not a directory, inv=map[]). The test throws err away. Flipping LoadInventory to silently skip a symlinked type dir stays green (wiki + pipeline). So the contract in the doc comment and the PR body ("a symlinked type dir is an error") has no test. Assert require.ErrorIs(err, brainfs.ErrNotRegular) and rename to match.
TestBackfillSkipsSymlinkedNotesAndASymlinkedRoot: a symlinked root aborts the backfill (assert.Error). It is not skipped. Rename it.
Verified by execution (holds)
Probe
Result
IndexDoc on a static outside link / in-brain file link / FIFO / .. escape
refused / refused / refused, no hang / path escapes from parent
Backfill with knowledge -> . or wiki -> raw
aborts, 0 raw hits
Backfill with a symlinked hall wiki/w/evilhall -> raw/facts
not descended, 0 raw hits
Backfill with a FIFO plus a mode-000 dir
FIFO skipped; aborts on the locked dir (n=1). main also aborted on walk errors
LoadInventory with an in-brain file link, a FIFO, a unix socket, or a dir named d.md
none listed, no hang
LoadInventory / RebuildIndex with wiki or wiki/concepts symlinked
error, nothing read
brainDir as a symlink (pod /app/brain shape): IndexDoc, Backfill, LoadInventory, RebuildIndex
all work
Flips: no handle check
red (hang) in graphsync and wiki
Flips: ReadDirRegular lists non-regular entries / no realDir
red
Flips: backfill ignores walk errors / skips a non-dir root
red
Non-blocking
LoadInventory(missingBrainDir) now returns an error. It used to return an empty inventory.brainfs.Open fails before the per-dir ErrNotExist skip. The live brain dir exists and startup refuses without it, so this is a behaviour change to note, not a breakage.
A symlinked wiki/<type> now aborts every pipeline.Run/RunRaw/BackfillRefs until the tripwire removes it (30 s or less). So a Syncthing peer can stall ingest. Before this PR it was followed. Fail-closed is the right direction. RebuildIndex failing after pages are written only becomes a warning, and index.md goes stale.
Startup: a backfill error from BackfillFromBrainDir is only logger.Warn (main.go:347). Not fatal, so the pod still starts.
IndexDoc is not limited to wiki//knowledge/, and follows in-brain intermediate dir links.
Probes: wiki/evil -> ../raw indexes wiki/evil/facts/secret.md (2 raw hits). wiki/w/../../raw/secret.md with no symlink at all gives the same.
brain_tunnel and brain_update pass caller paths through, and wingOf accepts ...
The intermediate-link half is #167-class. The .. half is pre-existing. A path check in IndexDoc (prefix wiki/ or knowledge/, no .., realDir(path.Dir(rel))) would close both cheaply.
Pre-existing: brain_tunnel writes outside the brain. Executed on temp files: WriteTunnel(b, "wiki/a/../../../outside.md", ...) modified a file outside the brain. Already tracked as #169.
Outside content still reaches the LLM prompts through pipeline readers this PR doesn't touch. Executed: symlinked wiki/sources/<slug>.md (via readSourcePage) plus schema.md (via loadSchema) put the sentinel into the prompt 10 times. The PR lists these as remaining readers (pipeline slice). Also BackfillRefs still uses os.ReadFile on wiki/sources/*.
Not checked
A real Syncthing peer.
The live pod or a live graph backfill. No BRAIN_GRAPH_BACKFILL run.
## pr-gatekeeper verdict: **BLOCK** (PR #168 @ c7a6429). Not merged.
CI run 9176 is green. Locally, `CI=1 task check` exits 0 with 0 FAILs, and `-race -count=5` on graphsync, wiki, brainfs and pipeline is green. **None of that covers the findings below.** All of them were found by running code: probes and flips in a scratch copy of the module. The worktree was not modified.
### Blocking
**1. Every new symlink test points at a file outside the brain, and os.Root refuses those anyway. So the Lstat layer this PR depends on has no test in either package.** This is the same class as the earlier rounds.
Each flip below was run against `./internal/graphsync/ ./internal/wiki/ ./internal/brainfs/`.
| Flip | Result |
|---|---|
| `indexDoc` reads through a no-Lstat reader (`readOpened`) | **green** |
| `readFrontmatter` reads through a no-Lstat reader | **green** |
| `pageFirstSentence` reads through a no-Lstat reader | **green** |
| `ReadDirRegular` Lstats only the last component (`wiki/` itself not checked) | **green** in all three packages. There is no brainfs unit test for `ReadDirRegular` at all. |
| `ReadRegular` drops its Lstat type check | graphsync and wiki green; only `brainfs.TestReadRegularRefusesSymlinksAndReadsRegularFiles` goes red |
These flips are not harmless. Under the indexDoc bypass, a static in-brain link `wiki/w/facts/inlink.md -> ../../../raw/secret.md` puts raw/'s title and wikilinks into the graph (`IndexDoc` err=nil, 2 sentinel hits). With the PR's code it is refused. **Fix:** add an in-brain-target case next to each outside-target case (a file link into `raw/`, and `wiki -> raw/w` for ReadDirRegular), plus a brainfs unit test for `ReadDirRegular`.
**2. Rule 9: `TestRebuildIndexDoesNotReadAPageSwappedForASymlink` and the two `...SurvivesSwapRaces` tests claim something the code doesn't do.** I ran the same swap harness (`brainfstest.StartSwap`) at HEAD, unmodified, with the link pointing **inside** the brain (`../../raw/s.md`) instead of outside it, 1.5 s each:
| Reader | Leaks of raw/ content |
|---|---|
| `RebuildIndex` → `wiki/index.md` | **8,229** in 55k iterations |
| `LoadInventory` → inventory (LLM prompts) | **9,528** in 93k |
| `IndexDoc` → graph | **109,472** in 232k |
**Cause:** `ReadRegular` Lstats the path and then opens it again by path. If the entry is swapped for an in-root symlink in between, os.Root follows it, and the handle check passes because the target is a regular file. This is a *file* swap inside `ReadRegular`. #167 accepts a *directory* swap during a walk, which is a different gap. The reasoning #167 gives (in-brain only, a microsecond window, the tripwire) probably applies here too, but nobody has decided that.
**Fix, either:**
- (a) Compare the Lstat result with the handle's stat using `os.SameFile` in `ReadRegular`/`ReadRegularMax`. I tested a sketch: all three races went to **0 leaks** (242k, 102k and 73k iterations). That fix also covers slices 1–3, which share `ReadRegular`.
- (b) Mathias explicitly extends #167 to cover file swaps, and the tests are renamed to say *outside the brain* (for example `...DoesNotReadAnOutsideFileSwappedIn`).
As the names stand, they are false.
**3. Rule 9: two tests say "Skips" when the code aborts.**
- `TestLoadInventorySkipsASymlinkedTypeDir`: the code *returns an error* and the whole inventory is lost (probe: `err=... walk root component wiki/concepts is not a directory`, `inv=map[]`). The test throws `err` away. Flipping LoadInventory to *silently skip* a symlinked type dir stays **green** (wiki + pipeline). So the contract in the doc comment and the PR body ("a symlinked type dir is an error") has no test. Assert `require.ErrorIs(err, brainfs.ErrNotRegular)` and rename to match.
- `TestBackfillSkipsSymlinkedNotesAndASymlinkedRoot`: a symlinked root *aborts* the backfill (`assert.Error`). It is not skipped. Rename it.
### Verified by execution (holds)
| Probe | Result |
|---|---|
| IndexDoc on a static outside link / in-brain file link / FIFO / `..` escape | refused / refused / refused, no hang / `path escapes from parent` |
| Backfill with `knowledge -> .` or `wiki -> raw` | aborts, 0 raw hits |
| Backfill with a symlinked hall `wiki/w/evilhall -> raw/facts` | not descended, 0 raw hits |
| Backfill with a FIFO plus a mode-000 dir | FIFO skipped; aborts on the locked dir (n=1). main also aborted on walk errors |
| LoadInventory with an in-brain file link, a FIFO, a unix socket, or a dir named `d.md` | none listed, no hang |
| LoadInventory / RebuildIndex with `wiki` or `wiki/concepts` symlinked | error, nothing read |
| brainDir as a symlink (pod `/app/brain` shape): IndexDoc, Backfill, LoadInventory, RebuildIndex | all work |
| Flips: no handle check | red (hang) in graphsync and wiki |
| Flips: ReadDirRegular lists non-regular entries / no realDir | red |
| Flips: backfill ignores walk errors / skips a non-dir root | red |
### Non-blocking
- **`LoadInventory(missingBrainDir)` now returns an error. It used to return an empty inventory.** `brainfs.Open` fails before the per-dir ErrNotExist skip. The live brain dir exists and startup refuses without it, so this is a behaviour change to note, not a breakage.
- **A symlinked `wiki/<type>` now aborts every `pipeline.Run`/`RunRaw`/`BackfillRefs`** until the tripwire removes it (30 s or less). So a Syncthing peer can stall ingest. Before this PR it was followed. Fail-closed is the right direction. `RebuildIndex` failing after pages are written only becomes a warning, and index.md goes stale.
- **Startup:** a backfill error from `BackfillFromBrainDir` is only `logger.Warn` (main.go:347). Not fatal, so the pod still starts.
- **`IndexDoc` is not limited to `wiki/`/`knowledge/`, and follows in-brain intermediate dir links.**
- Probes: `wiki/evil -> ../raw` indexes `wiki/evil/facts/secret.md` (2 raw hits). `wiki/w/../../raw/secret.md` with no symlink at all gives the same.
- `brain_tunnel` and `brain_update` pass caller paths through, and `wingOf` accepts `..`.
- The intermediate-link half is #167-class. The `..` half is pre-existing. A path check in `IndexDoc` (prefix `wiki/` or `knowledge/`, no `..`, `realDir(path.Dir(rel))`) would close both cheaply.
- **Pre-existing: `brain_tunnel` writes outside the brain.** Executed on temp files: `WriteTunnel(b, "wiki/a/../../../outside.md", ...)` modified a file outside the brain. Already tracked as **#169**.
- **Outside content still reaches the LLM prompts through pipeline readers this PR doesn't touch.** Executed: symlinked `wiki/sources/<slug>.md` (via `readSourcePage`) plus `schema.md` (via `loadSchema`) put the sentinel into the prompt **10 times**. The PR lists these as remaining readers (pipeline slice). Also `BackfillRefs` still uses `os.ReadFile` on `wiki/sources/*`.
### Not checked
- A real Syncthing peer.
- The live pod or a live graph backfill. No `BRAIN_GRAPH_BACKFILL` run.
- Hard links (accepted residual).
- The `wiki/index.md` write path (#158).
- Performance of opening a root per `IndexDoc` call.
Review round 1 on PR #168:
- ReadRegular/ReadRegularMax Lstat the path, then open it again by path;
os.Root follows an in-root final symlink even with O_NOFOLLOW, so a
symlink to another brain file swapped in between was read (8k-109k
raw/ leaks in RebuildIndex, LoadInventory, IndexDoc). The opened handle
must now be the same file as the Lstat (os.SameFile). This covers every
ReadRegular caller, slices 1-3 included.
- Every symlink test pointed outside the brain, which os.Root refuses on
its own. Each now has an in-brain case too: a static in-brain link per
reader, AssertNoLeakFrom with an in-brain target, and brainfs unit tests
for ReadDirRegular and the in-brain swap.
- brainfstest: swap links are relative (os.Root refuses every absolute
link, so an absolute in-brain target tested nothing), and each swapper
has its own temp name (two swappers on one path deadlocked on each
other's FIFO).
- IndexDoc indexes only a clean wiki/ or knowledge/ path (callers pass
caller-supplied paths).
- Rule 9: TestLoadInventoryReturnsErrNotRegularForASymlinkedTypeDir now
asserts the error; the backfill test says it aborts on a symlinked root.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local CI=1 task check at the worktree: EXIT=0, 0 FAILs.
-race -count=5 on graphsync, wiki, brainfs and pipeline: green.
Rule 9
I read each new or renamed test name against its assertions, and they match. One nit: "Never" in TestReadRegularNeverReturns... claims more than a 1.5 s sampled race; DoesNotReturn would be honest.
Non-blocking
Per-caller in-brain cases for slices 1–3: yes, please, as a follow-up issue.
Each of these merged callers reading through a no-Lstat, no-SameFile reader stays green: search Query (search.go:146), hydrate (search.go:297), vectorstore Sync (sync.go:145), the context excerpt (tools_context.go:190) and ReadNote (update.go:188). That is the round-1 gap class, in merged code.
The shared ReadRegular unit tests cover the primitive, but not a caller that moves off it. Not this PR's diff, so it does not block.
SameFile also refuses a legitimate temp+rename replacement that lands between the Lstat and the open (Syncthing writes that way). The effects:
readFrontmatter falls back to the slug as the title.
pageFirstSentence returns "".
IndexDoc logs a warning.
The startup backfill aborts that root.
vectorstore deletes the stale chunks and then skips the re-embed until the next sync.
All of these are transient and heal themselves. It is the right trade-off, but it is a new trigger.
IndexDoc now refuses any brain_update path outside wiki/ and knowledge/ (with a warning). This matches the backfill's scope.
Still open, unchanged from round 1:
An in-brain intermediate dir link is still followed by IndexDoc (wiki/evil -> ../raw, 2 raw hits). This is #167-class.
The pipeline's readSourcePage and loadSchema still put outside content into the LLM prompt (10 hits). This is the pipeline slice.
LoadInventory on a missing brainDir returns an error.
Cosmetic:AssertNoLeakFrom reports "outside file leaked" even when the target is inside the brain.
## pr-gatekeeper verdict, round 2: **PASS** (PR #168 @ d90ce66). Not merged.
This is a verdict only. The PR is risk/high and CD deploys ingestion on merge, so the merge is Mathias's call.
### Round-1 blockers: closed, verified by execution
| Round-1 finding | Probe at d90ce66 | Result |
|---|---|---|
| 1. Caller-level Lstat bypass stayed green | `indexDoc` / `readFrontmatter` / `pageFirstSentence` reading through a no-Lstat, no-SameFile reader | **red**, each one (IndexDoc 119k leaks, LoadInventory 28k, RebuildIndex 6–8k) |
| 1. `ReadDirRegular` checked only the last component | flip | **red** (`TestReadDirRegularListsOnlyRegularFilesAndRefusesSymlinkedComponents`) |
| 2. In-brain file swapped in behind a symlink | my round-1 race probes (target `../../raw/s.md`) | IndexDoc **0** in 134k iterations, LoadInventory **0** in 65k, RebuildIndex **0** in 62k (round 1: 109k / 9.5k / 8.2k) |
| 3. Rule 9 "Skips" names | LoadInventory silently skips a symlinked type dir | **red** (`...ReturnsErrNotRegularForASymlinkedTypeDir`). The backfill name now says Aborts, which is what it asserts |
### Flip tests (all red)
| Mutation | Red tests |
|---|---|
| SameFile check off | 4 tests in graphsync, wiki and brainfs; **20/20** failures over `-count=5` |
| SameFile only in ReadRegular, not ReadRegularMax | `TestReadRegularNeverReturnsAnInBrainFileSwappedInBehindASymlink` |
| ReadDirRegular lists non-regular entries | the inventory race test plus the ReadDirRegular unit test |
| IndexDoc without `path.Clean` | `TestIndexDocRefusesPathsOutsideWikiAndKnowledgeAndUncleanPaths` |
| IndexDoc without the wiki/knowledge prefix | same test |
### Round-1 probes re-run (hold)
- **Refused:** a static in-brain link, an outside link, a FIFO, and `..` paths (now refused by the scope check).
- **Abort, 0 raw hits:** backfill with `knowledge -> .` or `wiki -> raw`.
- **Not descended:** a symlinked hall.
- **LoadInventory and RebuildIndex:** a FIFO, a socket or a dir named `d.md` is not listed; a symlinked `wiki` or type dir gives ErrNotRegular.
- **A symlinked brainDir (the pod's `/app/brain` shape)** works for all four entry points.
### Gate and CI
- CI run 9180 on d90ce66: success.
- Local `CI=1 task check` at the worktree: EXIT=0, 0 FAILs.
- `-race -count=5` on graphsync, wiki, brainfs and pipeline: green.
### Rule 9
I read each new or renamed test name against its assertions, and they match. One nit: "Never" in `TestReadRegularNeverReturns...` claims more than a 1.5 s sampled race; `DoesNotReturn` would be honest.
### Non-blocking
- **Per-caller in-brain cases for slices 1–3: yes, please, as a follow-up issue.**
- Each of these merged callers reading through a no-Lstat, no-SameFile reader stays **green**: search Query (`search.go:146`), hydrate (`search.go:297`), vectorstore Sync (`sync.go:145`), the context excerpt (`tools_context.go:190`) and ReadNote (`update.go:188`). That is the round-1 gap class, in merged code.
- The shared `ReadRegular` unit tests cover the primitive, but not a caller that moves off it. Not this PR's diff, so it does not block.
- **SameFile also refuses a legitimate temp+rename replacement** that lands between the Lstat and the open (Syncthing writes that way). The effects:
- `readFrontmatter` falls back to the slug as the title.
- `pageFirstSentence` returns "".
- IndexDoc logs a warning.
- The startup backfill aborts that root.
- vectorstore deletes the stale chunks and then skips the re-embed until the next sync.
All of these are transient and heal themselves. It is the right trade-off, but it is a new trigger.
- **IndexDoc now refuses** any `brain_update` path outside wiki/ and knowledge/ (with a warning). This matches the backfill's scope.
- **Still open, unchanged from round 1:**
- An in-brain intermediate dir link is still followed by IndexDoc (`wiki/evil -> ../raw`, 2 raw hits). This is #167-class.
- The pipeline's `readSourcePage` and `loadSchema` still put outside content into the LLM prompt (10 hits). This is the pipeline slice.
- LoadInventory on a missing brainDir returns an error.
- **Cosmetic:** `AssertNoLeakFrom` reports "outside file leaked" even when the target is inside the brain.
### Not checked
- A real Syncthing peer.
- The live pod or a live graph backfill.
- Hard links (accepted residual).
- The `wiki/index.md` write path (#158).
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
mathias
Refs #157 (slice 4). risk/high: control code, adversarial review required.
Problem
Each of these read notes by path, so a symlinked note (Syncthing syncs symlinks) put the target's title, aliases and wikilinks into the place listed, and a FIFO hung the read:
graphsync.IndexDoc/BackfillFromBrainDir→ the knowledge graph (entities and edges)wiki.LoadInventory→ the wiki inventory, and the LLM prompts the pipeline builds from itwiki.RebuildIndex→ page summaries inwiki/index.mdChange
WalkRegular, skips a missing root, and aborts on any other walk error, including a symlinked root.brainfs.ReadDirRegularrequires every component of the dir to be a real directory, and lists regular files only. Frontmatter and first sentences are read through the root. A symlinked type dir is an error.Tests
All red before the fix: 186k–378k leaks in the races, and the static symlink cases failed. All have positive controls. The new
conceptstests initially usedconcept, and their positive controls caught that.Flips (red)
IndexDocreverted toos.ReadFileReadDirRegularlists non-regular entriesReadDirRegularwithoutrealDirNot flipped: inventory frontmatter through
os.ReadFile. The patch doesn't compile without a signature change.Not covered
RebuildIndexwriteswiki/index.mdwithos.WriteFile, which follows a symlink. That's a write path, tracked in #158.🤖 Generated with Claude Code
Created via git-mcp on behalf of @mathias
pr-gatekeeper verdict: BLOCK (PR #168 @
c7a6429). Not merged.CI run 9176 is green. Locally,
CI=1 task checkexits 0 with 0 FAILs, and-race -count=5on graphsync, wiki, brainfs and pipeline is green. None of that covers the findings below. All of them were found by running code: probes and flips in a scratch copy of the module. The worktree was not modified.Blocking
1. Every new symlink test points at a file outside the brain, and os.Root refuses those anyway. So the Lstat layer this PR depends on has no test in either package. This is the same class as the earlier rounds.
Each flip below was run against
./internal/graphsync/ ./internal/wiki/ ./internal/brainfs/.indexDocreads through a no-Lstat reader (readOpened)readFrontmatterreads through a no-Lstat readerpageFirstSentencereads through a no-Lstat readerReadDirRegularLstats only the last component (wiki/itself not checked)ReadDirRegularat all.ReadRegulardrops its Lstat type checkbrainfs.TestReadRegularRefusesSymlinksAndReadsRegularFilesgoes redThese flips are not harmless. Under the indexDoc bypass, a static in-brain link
wiki/w/facts/inlink.md -> ../../../raw/secret.mdputs raw/'s title and wikilinks into the graph (IndexDocerr=nil, 2 sentinel hits). With the PR's code it is refused. Fix: add an in-brain-target case next to each outside-target case (a file link intoraw/, andwiki -> raw/wfor ReadDirRegular), plus a brainfs unit test forReadDirRegular.2. Rule 9:
TestRebuildIndexDoesNotReadAPageSwappedForASymlinkand the two...SurvivesSwapRacestests claim something the code doesn't do. I ran the same swap harness (brainfstest.StartSwap) at HEAD, unmodified, with the link pointing inside the brain (../../raw/s.md) instead of outside it, 1.5 s each:RebuildIndex→wiki/index.mdLoadInventory→ inventory (LLM prompts)IndexDoc→ graphCause:
ReadRegularLstats the path and then opens it again by path. If the entry is swapped for an in-root symlink in between, os.Root follows it, and the handle check passes because the target is a regular file. This is a file swap insideReadRegular. #167 accepts a directory swap during a walk, which is a different gap. The reasoning #167 gives (in-brain only, a microsecond window, the tripwire) probably applies here too, but nobody has decided that.Fix, either:
os.SameFileinReadRegular/ReadRegularMax. I tested a sketch: all three races went to 0 leaks (242k, 102k and 73k iterations). That fix also covers slices 1–3, which shareReadRegular....DoesNotReadAnOutsideFileSwappedIn).As the names stand, they are false.
3. Rule 9: two tests say "Skips" when the code aborts.
TestLoadInventorySkipsASymlinkedTypeDir: the code returns an error and the whole inventory is lost (probe:err=... walk root component wiki/concepts is not a directory,inv=map[]). The test throwserraway. Flipping LoadInventory to silently skip a symlinked type dir stays green (wiki + pipeline). So the contract in the doc comment and the PR body ("a symlinked type dir is an error") has no test. Assertrequire.ErrorIs(err, brainfs.ErrNotRegular)and rename to match.TestBackfillSkipsSymlinkedNotesAndASymlinkedRoot: a symlinked root aborts the backfill (assert.Error). It is not skipped. Rename it.Verified by execution (holds)
..escapepath escapes from parentknowledge -> .orwiki -> rawwiki/w/evilhall -> raw/factsd.mdwikiorwiki/conceptssymlinked/app/brainshape): IndexDoc, Backfill, LoadInventory, RebuildIndexNon-blocking
LoadInventory(missingBrainDir)now returns an error. It used to return an empty inventory.brainfs.Openfails before the per-dir ErrNotExist skip. The live brain dir exists and startup refuses without it, so this is a behaviour change to note, not a breakage.wiki/<type>now aborts everypipeline.Run/RunRaw/BackfillRefsuntil the tripwire removes it (30 s or less). So a Syncthing peer can stall ingest. Before this PR it was followed. Fail-closed is the right direction.RebuildIndexfailing after pages are written only becomes a warning, and index.md goes stale.BackfillFromBrainDiris onlylogger.Warn(main.go:347). Not fatal, so the pod still starts.IndexDocis not limited towiki//knowledge/, and follows in-brain intermediate dir links.wiki/evil -> ../rawindexeswiki/evil/facts/secret.md(2 raw hits).wiki/w/../../raw/secret.mdwith no symlink at all gives the same.brain_tunnelandbrain_updatepass caller paths through, andwingOfaccepts.....half is pre-existing. A path check inIndexDoc(prefixwiki/orknowledge/, no..,realDir(path.Dir(rel))) would close both cheaply.brain_tunnelwrites outside the brain. Executed on temp files:WriteTunnel(b, "wiki/a/../../../outside.md", ...)modified a file outside the brain. Already tracked as #169.wiki/sources/<slug>.md(viareadSourcePage) plusschema.md(vialoadSchema) put the sentinel into the prompt 10 times. The PR lists these as remaining readers (pipeline slice). AlsoBackfillRefsstill usesos.ReadFileonwiki/sources/*.Not checked
BRAIN_GRAPH_BACKFILLrun.wiki/index.mdwrite path (#158).IndexDoccall.pr-gatekeeper verdict, round 2: PASS (PR #168 @
d90ce66). Not merged.This is a verdict only. The PR is risk/high and CD deploys ingestion on merge, so the merge is Mathias's call.
Round-1 blockers: closed, verified by execution
d90ce66indexDoc/readFrontmatter/pageFirstSentencereading through a no-Lstat, no-SameFile readerReadDirRegularchecked only the last componentTestReadDirRegularListsOnlyRegularFilesAndRefusesSymlinkedComponents)../../raw/s.md)...ReturnsErrNotRegularForASymlinkedTypeDir). The backfill name now says Aborts, which is what it assertsFlip tests (all red)
-count=5TestReadRegularNeverReturnsAnInBrainFileSwappedInBehindASymlinkpath.CleanTestIndexDocRefusesPathsOutsideWikiAndKnowledgeAndUncleanPathsRound-1 probes re-run (hold)
..paths (now refused by the scope check).knowledge -> .orwiki -> raw.d.mdis not listed; a symlinkedwikior type dir gives ErrNotRegular./app/brainshape) works for all four entry points.Gate and CI
d90ce66: success.CI=1 task checkat the worktree: EXIT=0, 0 FAILs.-race -count=5on graphsync, wiki, brainfs and pipeline: green.Rule 9
I read each new or renamed test name against its assertions, and they match. One nit: "Never" in
TestReadRegularNeverReturns...claims more than a 1.5 s sampled race;DoesNotReturnwould be honest.Non-blocking
Per-caller in-brain cases for slices 1–3: yes, please, as a follow-up issue.
search.go:146), hydrate (search.go:297), vectorstore Sync (sync.go:145), the context excerpt (tools_context.go:190) and ReadNote (update.go:188). That is the round-1 gap class, in merged code.ReadRegularunit tests cover the primitive, but not a caller that moves off it. Not this PR's diff, so it does not block.SameFile also refuses a legitimate temp+rename replacement that lands between the Lstat and the open (Syncthing writes that way). The effects:
readFrontmatterfalls back to the slug as the title.pageFirstSentencereturns "".All of these are transient and heal themselves. It is the right trade-off, but it is a new trigger.
IndexDoc now refuses any
brain_updatepath outside wiki/ and knowledge/ (with a warning). This matches the backfill's scope.Still open, unchanged from round 1:
wiki/evil -> ../raw, 2 raw hits). This is #167-class.readSourcePageandloadSchemastill put outside content into the LLM prompt (10 hits). This is the pipeline slice.Cosmetic:
AssertNoLeakFromreports "outside file leaked" even when the target is inside the brain.Not checked
wiki/index.mdwrite path (#158).