Refs #157 (slice 3 of the reader migration). risk/medium: control code, adversarial review required (rule 8).
Problem
vectorstore.Sync walked wiki/ and knowledge/ with filepath.WalkDir and read each .md with os.ReadFile. A symlinked note (Syncthing syncs symlinks into /var/lib/supervisor/brain) had its target embedded into pgvector and served by semantic search. A FIFO stalled the sync.
Change
Walk via brainfs.WalkRegular, read via brainfs.ReadRegular: regular files only, type checked on the opened handle.
brainfs.Lstat added for the mtime staleness check.
Walk errors still abort before the delete pass, and a missing scan dir is still skipped.
Live effect on deploy
Chunks already embedded from a symlinked note are deleted on the first sync, because the note is no longer seen. That is the intended purge.
Tests and flips
TestSyncRefusesSymlinksAndSurvivesSwapRaces: static symlink plus a positive control, and brainfstest leak/hang races. Red before the fix (static leak, 139k leaks in the race).
os.Stat instead of bfs.Lstat → stays green, by design: the walk has already filtered to regular files, so the stat only decides re-embed timing, and the read guard is the control.
Not covered
Hard links (accepted residual, see brainfs).
Other #157 readers: graphsync, wiki inventory, brain index/tunnel/linkcheck, pipeline, provenance, frontmatter scan.
Refs #157 (slice 3 of the reader migration). risk/medium: control code, adversarial review required (rule 8).
## Problem
`vectorstore.Sync` walked `wiki/` and `knowledge/` with `filepath.WalkDir` and read each `.md` with `os.ReadFile`. A symlinked note (Syncthing syncs symlinks into `/var/lib/supervisor/brain`) had its **target embedded into pgvector and served by semantic search**. A FIFO stalled the sync.
## Change
- Walk via `brainfs.WalkRegular`, read via `brainfs.ReadRegular`: regular files only, type checked on the opened handle.
- `brainfs.Lstat` added for the mtime staleness check.
- Walk errors still abort before the delete pass, and a missing scan dir is still skipped.
## Live effect on deploy
Chunks already embedded from a symlinked note are **deleted** on the first sync, because the note is no longer seen. That is the intended purge.
## Tests and flips
- `TestSyncRefusesSymlinksAndSurvivesSwapRaces`: static symlink plus a positive control, and brainfstest leak/hang races. Red before the fix (static leak, 139k leaks in the race).
- `TestSyncDeletesChunksOfASymlinkedNote`
- `TestSyncAbortsBeforeDeletingWhenASubdirIsUnreadable`
- `TestLstatDoesNotFollowSymlinksAndRefusesEscapes`
Flips:
- `os.ReadFile` instead of `ReadRegular` → red
- walk error dropped → red
- onErr swallowing every error → red
- missing-dir skip removed → red
- `os.Stat` instead of `bfs.Lstat` → **stays green, by design**: the walk has already filtered to regular files, so the stat only decides re-embed timing, and the read guard is the control.
## Not covered
- Hard links (accepted residual, see brainfs).
- Other #157 readers: graphsync, wiki inventory, brain index/tunnel/linkcheck, pipeline, provenance, frontmatter scan.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---
_Created via git-mcp on behalf of @mathias_
Sync walked wiki/ and knowledge/ with filepath.WalkDir and read each .md
with os.ReadFile. A symlinked note (Syncthing syncs symlinks) had its
target embedded into pgvector and served by semantic search; a FIFO
stalled the sync. The walk and read now go through brainfs: regular
files only, type checked on the opened handle.
- Chunks already embedded from a symlinked note are deleted on the next
sync, because the note is no longer seen.
- A walk error still aborts Sync before the delete pass, so an unreadable
subdir does not delete chunks of files that exist. A missing scan dir
is skipped as before.
- brainfs gains Lstat, used for the mtime staleness check.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pr-gatekeeper verdict: BLOCK (PR #166 @ 74a9621). Not merged.
CI run 9058 (CI / Lint / Test / Vet) is green. Locally, CI=1 task check gave EXIT=0 with 0 FAILs, and -race -count=10 on vectorstore and brainfs is green. None of that covers the finding below.
Blocking
ingestion/internal/vectorstore/sync.go (the bfs.WalkRegular(sub, ...) call) together with brainfs.WalkRegular: a symlinked scan root is now followed. main did not follow it.
fs.WalkDir stats its root with fs.Stat, which follows symlinks. os.Root allows that as long as the target stays inside the brain. So if knowledge or wiki is a symlink to somewhere else in the brain, Sync walks the target.
filepath.WalkDir on main Lstat'ed the root. It saw a non-directory and did not descend.
With knowledge -> ., Sync now embeds every .md in the brain under a knowledge/ alias: sessions/ (in .stignore, so koala-local), raw/ (the inbox), and root files such as CLAUDE.md.
Search then serves those hits. hydrate reads knowledge/sessions/x.md through ReadRegular, which follows the in-root intermediate symlink.
Repro (executed). Brain contents: wiki/w/a.md, sessions/s1.md, raw/r1.md and CLAUDE.md, each with its own sentinel, plus knowledge -> .. Then vectorstore.Sync with a recording embedder:
Nothing leaves the brain. Every escape probe below held.
This is still the #157 vector: a Syncthing-delivered symlink that a reader follows. It is a regression against main in the reader this PR hardens, and it contradicts WalkRegular's doc ("symlinks ... are skipped, never followed").
The symlinkguard tripwire limits exposure: it removes the link within 30 s, and the next Sync purges the alias rows (≤5 min). But this layer exists to make the guarantee not depend on the tripwire.
Fix (a correction, not a judgement call): Lstat the scan root through the root and require a real directory. If it is missing, skip as today. If it is a symlink or not a directory, either skip it or return an error. Then pin it with a test that has a symlinked knowledge pointing inside the brain and asserts nothing outside knowledge/ is embedded. Doing this in WalkRegular instead also fixes the other readers.
Choose the symlinked-root behaviour deliberately. "Skip as missing" deletes every chunk under that dir. "Error" stops the sync until the link is removed, which is what an outside-pointing root already does today (probe A).
Non-blocking
The rel == sub restriction in onErr is unpinned. Flip: change rel == sub && errors.Is(err, fs.ErrNotExist) to errors.Is(err, fs.ErrNotExist). The suite stays green. The PR body says "onErr swallowing every error → red", but swallowing every not-exist error is not covered. Effect: a subdir that vanishes mid-walk (Syncthing delete or rename) would be skipped and its chunks deleted, instead of aborting the run. In practice that means one tick of re-embed churn. Add a test.
The swap-race half of TestSyncRefusesSymlinksAndSurvivesSwapRaces has no positive control of its own. Its sync closure discards the error (_, _ = vectorstore.Sync), and AssertNoLeak does not require that the inside note was ever read. The first half's positive control and the red no-handle-check and no-O_NONBLOCK flips show the race is exercised today, so this is not decoration now. It would become decoration if Sync started erroring early under the swap.
An outside-pointing, or absolute in-brain, wiki/knowledge symlink aborts every Sync (statat wiki: path escapes from parent). knowledge/ is then never synced either. This fails closed and is logged, and the tripwire heals it. Recording the behaviour so it is not a surprise.
A dangling relative wiki symlink deletes every wiki/ chunk (ErrNotExist on the root, which is skipped). Same behaviour as main, so not a regression.
Labels. The issue is risk/high. The PR body says risk/medium and the PR has no labels. The label decides the lane.
Follow-up for slice 1 (not this PR):hydrate reads vector-hit paths with ReadRegular, which follows in-root intermediate symlinks. Even after this fix, a stale alias row resolves while the link exists.
Probes run (by execution, scratch copy of the module)
Live layout check: wiki and knowledge under /var/lib/supervisor/brain are real directories with 0 symlinks and 0 non-regular entries. The deploy itself would not break.
Flip tests
Mutation
Result
os.ReadFile instead of ReadRegular
red
walk error dropped
red
missing-root skip removed
red (13 tests)
seenParents set only after a successful read
red
WalkRegular descends into or yields symlinks
red (TestSyncDeletesChunksOfASymlinkedNote)
handle type check removed
red
O_NONBLOCK removed
red
brainfs.Lstat → root.Stat
red in brainfs (the unit test); green in vectorstore, which matches the author's reasoning, and I agree
all not-exist errors swallowed
green (non-blocking 1)
Rule 9:TestSyncDeletesChunksOfASymlinkedNote, TestSyncAbortsBeforeDeletingWhenASubdirIsUnreadable and TestLstatDoesNotFollowSymlinksAndRefusesEscapes match their assertions. TestSyncRefusesSymlinksAndSurvivesSwapRaces covers only a symlinked file. With the blocking finding open, "RefusesSymlinks" claims more than the code does for a symlinked scan root. Either narrow the name or add the root case.
Not checked
Real character or block device files: creating them needs root. The DT_CHR skip and the handle check are covered by reading the code and by the FIFO race.
Behaviour against a real Syncthing peer.
The CI job logs: I confirmed only the commit status (success in 40 s), not which packages it ran.
The PGStore SQL path: this PR does not touch it.
## pr-gatekeeper verdict: **BLOCK** (PR #166 @ 74a9621). Not merged.
CI run 9058 (`CI / Lint / Test / Vet`) is green. Locally, `CI=1 task check` gave EXIT=0 with 0 FAILs, and `-race -count=10` on vectorstore and brainfs is green. **None of that covers the finding below.**
### Blocking
**`ingestion/internal/vectorstore/sync.go` (the `bfs.WalkRegular(sub, ...)` call) together with `brainfs.WalkRegular`: a symlinked scan root is now followed. `main` did not follow it.**
- `fs.WalkDir` stats its root with `fs.Stat`, which follows symlinks. `os.Root` allows that as long as the target stays inside the brain. So if `knowledge` or `wiki` is a symlink to somewhere else in the brain, Sync walks the target.
- `filepath.WalkDir` on `main` Lstat'ed the root. It saw a non-directory and did not descend.
- With `knowledge -> .`, Sync now embeds every `.md` in the brain under a `knowledge/` alias: `sessions/` (in `.stignore`, so koala-local), `raw/` (the inbox), and root files such as `CLAUDE.md`.
- Search then serves those hits. `hydrate` reads `knowledge/sessions/x.md` through `ReadRegular`, which follows the in-root intermediate symlink.
Repro (executed). Brain contents: `wiki/w/a.md`, `sessions/s1.md`, `raw/r1.md` and `CLAUDE.md`, each with its own sentinel, plus `knowledge -> .`. Then `vectorstore.Sync` with a recording embedder:
| | session sentinel | raw sentinel | root sentinel | upserts |
|---|---|---|---|---|
| PR @ 74a9621 | 1 | 1 | 1 | `knowledge/CLAUDE.md`, `knowledge/raw/r1.md`, `knowledge/sessions/s1.md`, `knowledge/wiki/w/a.md`, `wiki/w/a.md` |
| `origin/main` sync.go | 0 | 0 | 0 | `wiki/w/a.md` |
- Nothing leaves the brain. Every escape probe below held.
- This is still the #157 vector: a Syncthing-delivered symlink that a reader follows. It is a regression against `main` in the reader this PR hardens, and it contradicts `WalkRegular`'s doc ("symlinks ... are skipped, never followed").
- The symlinkguard tripwire limits exposure: it removes the link within 30 s, and the next Sync purges the alias rows (≤5 min). But this layer exists to make the guarantee not depend on the tripwire.
- Fix (a correction, not a judgement call): `Lstat` the scan root through the root and require a real directory. If it is missing, skip as today. If it is a symlink or not a directory, either skip it or return an error. Then pin it with a test that has a symlinked `knowledge` pointing inside the brain and asserts nothing outside `knowledge/` is embedded. Doing this in `WalkRegular` instead also fixes the other readers.
- **Choose the symlinked-root behaviour deliberately.** "Skip as missing" deletes every chunk under that dir. "Error" stops the sync until the link is removed, which is what an outside-pointing root already does today (probe A).
### Non-blocking
1. **The `rel == sub` restriction in `onErr` is unpinned.** Flip: change `rel == sub && errors.Is(err, fs.ErrNotExist)` to `errors.Is(err, fs.ErrNotExist)`. The suite stays **green**. The PR body says "onErr swallowing every error → red", but swallowing every *not-exist* error is not covered. Effect: a subdir that vanishes mid-walk (Syncthing delete or rename) would be skipped and its chunks deleted, instead of aborting the run. In practice that means one tick of re-embed churn. Add a test.
2. **The swap-race half of `TestSyncRefusesSymlinksAndSurvivesSwapRaces` has no positive control of its own.** Its `sync` closure discards the error (`_, _ = vectorstore.Sync`), and `AssertNoLeak` does not require that the inside note was ever read. The first half's positive control and the red no-handle-check and no-O_NONBLOCK flips show the race is exercised today, so this is not decoration now. It would become decoration if Sync started erroring early under the swap.
3. **An outside-pointing, or absolute in-brain, `wiki`/`knowledge` symlink aborts every Sync** (`statat wiki: path escapes from parent`). `knowledge/` is then never synced either. This fails closed and is logged, and the tripwire heals it. Recording the behaviour so it is not a surprise.
4. **A dangling relative `wiki` symlink deletes every `wiki/` chunk** (`ErrNotExist` on the root, which is skipped). Same behaviour as `main`, so not a regression.
5. **Labels.** The issue is `risk/high`. The PR body says risk/medium and the PR has no labels. The label decides the lane.
6. **Follow-up for slice 1 (not this PR):** `hydrate` reads vector-hit paths with `ReadRegular`, which follows in-root intermediate symlinks. Even after this fix, a stale alias row resolves while the link exists.
### Probes run (by execution, scratch copy of the module)
| Probe | Leak | Result |
|---|---|---|
| A: `wiki -> outside dir` | 0 | Sync aborts, 0 deletes |
| B: `wiki -> other` (relative, inside) | 0 | followed (see blocking finding) |
| B2: `wiki -> /abs/brain/other` | 0 | aborts (escape) |
| C: `wiki/w -> outside` | 0 | skipped; `wiki/w/*` chunks deleted |
| D: `wiki/w -> ../knowledge` | 0 | skipped; chunks deleted |
| E: FIFO, FIFO with writer held, unix socket, `-> /dev/zero`, `-> /dev/null` | 0 | no hang; stale FIFO and symlink chunks purged |
| G/M: `knowledge -> .` | 0 outside, **3 in-brain aliases** | **blocking finding** |
| H: brain dir itself a symlink | 0 | works (OpenRoot follows it, as intended) |
| I: note is a symlink to another in-brain note | 0 | skipped; chunk purged |
| J: `wiki` swapped between a real dir and a symlink to outside, about 127k Syncs | 0 | 72k aborts, 0 wrong deletes of `knowledge/` chunks |
| K: `wiki/w` (intermediate) swapped between a dir and a symlink to outside, about 116k Syncs | 0 | 543 inside reads, so the race was live |
| Real brain, read-only Sync with a stub store (counts only) | n/a | err=nil, 1532 parents = 1532 regular non-index `.md` on disk; 0 item errors |
Live layout check: `wiki` and `knowledge` under `/var/lib/supervisor/brain` are real directories with 0 symlinks and 0 non-regular entries. The deploy itself would not break.
### Flip tests
| Mutation | Result |
|---|---|
| `os.ReadFile` instead of `ReadRegular` | red |
| walk error dropped | red |
| missing-root skip removed | red (13 tests) |
| `seenParents` set only after a successful read | red |
| `WalkRegular` descends into or yields symlinks | red (`TestSyncDeletesChunksOfASymlinkedNote`) |
| handle type check removed | red |
| `O_NONBLOCK` removed | red |
| `brainfs.Lstat` → `root.Stat` | red in brainfs (the unit test); green in vectorstore, which matches the author's reasoning, and I agree |
| **all not-exist errors swallowed** | **green** (non-blocking 1) |
**Rule 9:** `TestSyncDeletesChunksOfASymlinkedNote`, `TestSyncAbortsBeforeDeletingWhenASubdirIsUnreadable` and `TestLstatDoesNotFollowSymlinksAndRefusesEscapes` match their assertions. `TestSyncRefusesSymlinksAndSurvivesSwapRaces` covers only a symlinked *file*. With the blocking finding open, "RefusesSymlinks" claims more than the code does for a symlinked scan root. Either narrow the name or add the root case.
### Not checked
- Real character or block device files: creating them needs root. The `DT_CHR` skip and the handle check are covered by reading the code and by the FIFO race.
- Behaviour against a real Syncthing peer.
- The CI job logs: I confirmed only the commit status (success in 40 s), not which packages it ran.
- The PGStore SQL path: this PR does not touch it.
Review round 1 on PR #166: fs.WalkDir stats its root with a follow, and
os.Root allows a symlink that stays inside the brain, so knowledge -> .
aliased raw/, sessions/ and root files into the embeddings (and a
wiki/<w> -> .. root into search results). On main, filepath.WalkDir
Lstat'ed the root and did not descend.
- WalkRegular Lstats its root and requires a real directory; anything
else goes to onErr as ErrNotRegular and nothing is yielded.
- Sync skips a missing scan root via bfs.Lstat before walking, and every
other walk error, including a symlinked root, aborts before the delete
pass. This removes the unpinned rel == sub special case.
- Search warns and skips a symlinked root (new test).
- Test renamed to TestSyncRefusesSymlinkedNotesAndSurvivesSwapRaces.
- wrapAfterStat comment: brainfs tests must not use t.Parallel.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The round-1 blocker is closed for vectorstore.Sync, verified by execution. Search's new root check misses one case, shown below.
CI run 9059 is green.
Locally, CI=1 task check gave EXIT=0 with 0 FAILs, and the worktree is clean.
-race -count=10 on vectorstore, brainfs and search is green.
None of that covers the finding below.
Blocking
ingestion/internal/brainfs/brainfs.goWalkRegular: the root check Lstats only the last component of relDir. A symlink in an earlier component is still followed, so search with Wing+Hall walks an aliased directory.
os.Root.Lstat("wiki/alias/facts") follows alias. The final component facts is a real directory, so the check passes and fs.WalkDir walks the alias target.
Repro (executed, static, no race): a brain with raw/facts/r.md containing zqrawhall, plus wiki/alias -> ../raw.
The same query with Wing only → hits=0, which is what the new test checks.
The aim of this round is to "require a real directory first", and for multi-component roots that doesn't hold yet. This is the round-1 blocker class (an in-brain alias through a symlinked walk root) reached one level up.
TestQueryDoesNotFollowASymlinkedSearchRootInsideTheBrain covers only a single-component wing root. Rule 9: the name claims the search root, and with a hall the root is wiki/<wing>/<hall>.
Reachability today is low. The target needs a subdirectory named after a hall (facts, decisions, failures, hypotheses or sources). The non-synced dirs (sessions/) have none, so in practice it aliases content a peer could already see. But the fix is a correction:
In WalkRegular, Lstat every prefix of relDir (wiki, wiki/alias, wiki/alias/facts) and require each to be a real directory.
Add a Wing+Hall case to the search test.
Sync is unaffected: its scan roots are single components.
Escalate to Mathias (accept or fix; this is a judgement call, not a correction)
A directory swapped for an in-brain symlink between listing and reading is followed. This applies at every depth, and also around the new root check.os.Root keeps reads inside the brain, but not inside the scan subtree.
Executed: about 210k Syncs per probe while a swapper toggles a path between a real dir and a symlink. Session file sessions/s1.md carries a sentinel.
Swapped path
link target
session sentinel embedded
knowledge (scan root, TOCTOU on the new Lstat)
.
1
knowledge
sessions
4
wiki/w (subdir)
../sessions
10,412 (16,722 alias upserts)
Mechanism:fs.WalkDir sees w as a dir, then calls ReadDir("wiki/w") by path, which follows a swapped-in symlink. ReadRegular("wiki/w/s1.md") follows the intermediate the same way.
Not a regression:main (filepath.WalkDir + os.ReadFile) has the same race and the static file-symlink follow besides.
Reachability for the stated threat (a Syncthing peer) is poor. The swap must land inside a microsecond window of a walk that runs every 5 min. A local uid-1000 process can hit it, but it can already read sessions/. pgvector stores vectors, not text. The tripwire removes the link within 30 s.
The real fix is a design change: walk fd-relative. Open each directory through the root, compare it with os.SameFile against the Lstat result, and read relative to that handle. That is not a one-line correction.
Also: WalkRegular's doc ("symlinks ... never followed") overstates the guarantee under a race. Either qualify it, or record this as an accepted residual on #157 next to the hard-link one.
Round-1 items: verified
Item
Result
knowledge -> .
Sync aborts; 0 session/raw/root text embedded; no knowledge/ upserts; 0 deletes
wiki pointing outside, absolute-inside, relative-inside, dangling absolute or dangling relative
abort, 0 deletes. A dangling relative wiki used to delete every wiki chunk, so that is an improvement
Missing scan root
skipped, other root syncs
Subdir symlink inside or outside; note symlinked to an in-brain note; FIFO, held-writer FIFO, socket, links to /dev/*
0 leak, no hang, chunks purged
Outside-directed swap races (wiki about 438k Syncs, wiki/w about 243k)
Root error not passed to onErr: brainfs and vectorstore red.
Sync skipping (instead of aborting on) a symlinked root: TestSyncAbortsOnASymlinkedScanRoot red.
Missing-root pre-skip removed: 14 tests red, including TestSyncSkipsAMissingScanRoot.
onErr swallowing every error: ...SubdirIsUnreadable and ...SymlinkedScanRoot red.
None of my flips survived. The gap above is a case no test exercises, not a flip that stays green.
Rule 9: the brainfs and vectorstore names now match their assertions; TestSyncRefusesSymlinkedNotesAndSurvivesSwapRaces is honest. The search name is addressed above. The missing per-iteration positive control on the race half is not blocking: the handle-check and O_NONBLOCK flips show the race is live.
Cosmetic
The root error reads "walk root is not a directory: not a regular file", because ErrNotRegular is wrapped for a directory check. A sentinel such as ErrNotDir would read better.
Not checked
Real device nodes (they need root).
A real Syncthing peer.
CI job logs (commit status only).
Other WalkRegular callers besides Sync and search: none are in this diff.
## pr-gatekeeper verdict, round 2: **BLOCK** (PR #166 @ 66b6cf8). Not merged.
The round-1 blocker is closed for `vectorstore.Sync`, verified by execution. Search's new root check misses one case, shown below.
- CI run 9059 is green.
- Locally, `CI=1 task check` gave EXIT=0 with 0 FAILs, and the worktree is clean.
- `-race -count=10` on vectorstore, brainfs and search is green.
- None of that covers the finding below.
### Blocking
**`ingestion/internal/brainfs/brainfs.go` `WalkRegular`: the root check Lstats only the *last* component of `relDir`. A symlink in an earlier component is still followed, so search with Wing+Hall walks an aliased directory.**
- `os.Root.Lstat("wiki/alias/facts")` follows `alias`. The final component `facts` is a real directory, so the check passes and `fs.WalkDir` walks the alias target.
- Repro (executed, static, no race): a brain with `raw/facts/r.md` containing `zqrawhall`, plus `wiki/alias -> ../raw`.
- `Query(Query:"zqrawhall", Wing:"alias", Hall:"facts")` → err=nil, **hits=1**.
- The same query with Wing only → hits=0, which is what the new test checks.
- The aim of this round is to "require a real directory first", and for multi-component roots that doesn't hold yet. This is the round-1 blocker class (an in-brain alias through a symlinked walk root) reached one level up.
- `TestQueryDoesNotFollowASymlinkedSearchRootInsideTheBrain` covers only a single-component wing root. Rule 9: the name claims the search root, and with a hall the root is `wiki/<wing>/<hall>`.
- Reachability today is low. The target needs a subdirectory named after a hall (facts, decisions, failures, hypotheses or sources). The non-synced dirs (`sessions/`) have none, so in practice it aliases content a peer could already see. But the fix is a correction:
- In `WalkRegular`, Lstat every prefix of `relDir` (`wiki`, `wiki/alias`, `wiki/alias/facts`) and require each to be a real directory.
- Add a Wing+Hall case to the search test.
- Sync is unaffected: its scan roots are single components.
### Escalate to Mathias (accept or fix; this is a judgement call, not a correction)
**A directory swapped for an in-brain symlink between listing and reading is followed. This applies at every depth, and also around the new root check.** `os.Root` keeps reads inside the brain, but not inside the scan subtree.
Executed: about 210k Syncs per probe while a swapper toggles a path between a real dir and a symlink. Session file `sessions/s1.md` carries a sentinel.
| Swapped path | link target | session sentinel embedded |
|---|---|---|
| `knowledge` (scan root, TOCTOU on the new Lstat) | `.` | 1 |
| `knowledge` | `sessions` | 4 |
| `wiki/w` (subdir) | `../sessions` | **10,412** (16,722 alias upserts) |
- **Mechanism:** `fs.WalkDir` sees `w` as a dir, then calls `ReadDir("wiki/w")` by path, which follows a swapped-in symlink. `ReadRegular("wiki/w/s1.md")` follows the intermediate the same way.
- **Not a regression:** `main` (filepath.WalkDir + os.ReadFile) has the same race and the static file-symlink follow besides.
- **Reachability for the stated threat** (a Syncthing peer) is poor. The swap must land inside a microsecond window of a walk that runs every 5 min. A local uid-1000 process can hit it, but it can already read `sessions/`. pgvector stores vectors, not text. The tripwire removes the link within 30 s.
- **The real fix is a design change:** walk fd-relative. Open each directory through the root, compare it with `os.SameFile` against the Lstat result, and read relative to that handle. That is not a one-line correction.
- Also: `WalkRegular`'s doc ("symlinks ... never followed") overstates the guarantee under a race. Either qualify it, or record this as an accepted residual on #157 next to the hard-link one.
### Round-1 items: verified
| Item | Result |
|---|---|
| `knowledge -> .` | Sync aborts; 0 session/raw/root text embedded; no `knowledge/` upserts; 0 deletes |
| `wiki` pointing outside, absolute-inside, relative-inside, dangling absolute or dangling relative | abort, 0 deletes. A dangling relative `wiki` used to delete every wiki chunk, so that is an improvement |
| Missing scan root | skipped, other root syncs |
| Subdir symlink inside or outside; note symlinked to an in-brain note; FIFO, held-writer FIFO, socket, links to `/dev/*` | 0 leak, no hang, chunks purged |
| Outside-directed swap races (`wiki` about 438k Syncs, `wiki/w` about 243k) | 0 leaks, 0 wrong deletes |
| Real brain, read-only, stub store | err=nil, 1532 parents = 1532 regular non-index `.md` |
| Labels | `risk/high` |
### Flip tests
These went red:
- `WalkRegular` root `Lstat` → `Stat`: brainfs, vectorstore and search tests red.
- Root `IsDir` check removed: all three red.
- Root error not passed to onErr: brainfs and vectorstore red.
- Sync skipping (instead of aborting on) a symlinked root: `TestSyncAbortsOnASymlinkedScanRoot` red.
- Missing-root pre-skip removed: 14 tests red, including `TestSyncSkipsAMissingScanRoot`.
- onErr swallowing every error: `...SubdirIsUnreadable` and `...SymlinkedScanRoot` red.
None of my flips survived. The gap above is a case no test exercises, not a flip that stays green.
**Rule 9:** the brainfs and vectorstore names now match their assertions; `TestSyncRefusesSymlinkedNotesAndSurvivesSwapRaces` is honest. The search name is addressed above. The missing per-iteration positive control on the race half is **not blocking**: the handle-check and O_NONBLOCK flips show the race is live.
### Cosmetic
The root error reads "walk root is not a directory: not a regular file", because `ErrNotRegular` is wrapped for a directory check. A sentinel such as `ErrNotDir` would read better.
### Not checked
- Real device nodes (they need root).
- A real Syncthing peer.
- CI job logs (commit status only).
- Other `WalkRegular` callers besides Sync and search: none are in this diff.
Review round 2 on PR #166: Lstat on the walk root checks only its last
component, so search with Wing+Hall (root wiki/<wing>/<hall>) walked a
symlinked <wing> whose target had a real <hall> directory.
WalkRegular now Lstats every prefix of the root and requires each to be
a real directory. New brainfs and search (Wing+Hall) tests; the Wing-only
search test is renamed to say what it covers.
The doc now records the accepted residual: a directory swapped for a
symlink between listing and read is still followed; os.Root keeps the
read inside the brain but not inside the walk root.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This is a verdict only. The PR is risk/high and CD deploys on merge, so the merge is Mathias's call. The race residual is going to him separately; see the end.
Round-2 blocker: closed, verified by execution
realDir Lstats every prefix of the walk root. There are two WalkRegular callers (found by grep, non-test): search.Query and vectorstore.Sync. Both are covered below.
wiki -> raw: unscoped query, and Wing=facts under it
0 / 0
Wing ../raw (traversal)
0
Sync: knowledge -> .; wiki pointing outside, absolute-inside, relative-inside or dangling
abort, 0 embedded, 0 deletes
Sync: subdir or note symlinks; FIFO, held-writer FIFO, socket, links to /dev/*
0 leak, no hang, chunks purged
Outside-directed swap races (wiki about 409k Syncs, wiki/w about 230k)
0 leaks, 0 wrong deletes
Positive controls on the real brain (read-only, counts only):
Query, Wing+Hall on a real wing's facts: 27 hits, equal to the 27 matching files on disk.
Wing-only: 66 hits. Unscoped: at the limit.
Sync with a stub store: err=nil, 1532 parents = 1532 regular non-index .md.
The prefix check does not suppress legitimate scoped search.
Flip tests: all red
Mutation
Red
Check only the full path (old behaviour)
both new tests
Check only the first component
both search wing tests
Lstat → Stat in realDir
5 tests across brainfs, search and vectorstore
IsDir check dropped
the same 5
rel == "."-style bypass widened to wiki*
both search wing tests
The round-1 and round-2 flips are unchanged and were red at 66b6cf8. sync.go has not changed since.
Rule 9:TestWalkRegularReportsASymlinkedIntermediateComponentOfTheWalkRoot, TestQueryDoesNotFollowASymlinkedWingRootInsideTheBrain and TestQueryDoesNotFollowASymlinkedWingUnderAWingAndHallRoot match their assertions.
Doc wording on the residual: accurate
realDir says "a swap after the check is not caught". WalkRegular says a directory swapped between listing and read is followed, and that os.Root keeps the read inside the brain but not inside relDir. Both match what I measured:
Root swap after the check (knowledge ↔ symlink to sessions): 9 of about 202k Syncs embedded session text.
Subdir swap (wiki/w ↔ ../sessions): 10,597 of about 206k.
0 outside-brain leaks in every race.
That is the residual exactly as stated. Accepting it or redesigning (an fd-relative walk) is Mathias's decision. It is not part of this verdict.
Local CI=1 task check: EXIT=0, 0 FAILs. The worktree is clean.
-race -count=10 on vectorstore, brainfs and search: green.
Non-blocking
The two new search tests only assert empty results, with no in-test positive control. The flips going red, and the real-brain scoped query returning 27/27, show the path is live.
The error text "walk root component X is not a directory: not a regular file" reads oddly. An ErrNotDir sentinel would read better.
Not checked
Real device nodes (they need root).
A real Syncthing peer.
CI job logs (commit status only).
## pr-gatekeeper verdict, round 3: **PASS** (PR #166 @ 31071bc). Not merged.
This is a verdict only. The PR is `risk/high` and CD deploys on merge, so the merge is Mathias's call. The race residual is going to him separately; see the end.
### Round-2 blocker: closed, verified by execution
`realDir` Lstats every prefix of the walk root. There are two `WalkRegular` callers (found by grep, non-test): `search.Query` and `vectorstore.Sync`. Both are covered below.
| Probe (static) | Result |
|---|---|
| `wiki/alias -> ../raw`, Wing=alias, Hall=facts (my round-2 repro) | hits=0 (was 1) |
| Wing real, hall `wiki/real/facts -> ../../raw` | hits=0 |
| `wiki -> raw`: unscoped query, and Wing=facts under it | 0 / 0 |
| Wing `../raw` (traversal) | 0 |
| Sync: `knowledge -> .`; `wiki` pointing outside, absolute-inside, relative-inside or dangling | abort, 0 embedded, 0 deletes |
| Sync: subdir or note symlinks; FIFO, held-writer FIFO, socket, links to `/dev/*` | 0 leak, no hang, chunks purged |
| Outside-directed swap races (`wiki` about 409k Syncs, `wiki/w` about 230k) | 0 leaks, 0 wrong deletes |
**Positive controls on the real brain (read-only, counts only):**
- `Query`, Wing+Hall on a real wing's `facts`: 27 hits, equal to the 27 matching files on disk.
- Wing-only: 66 hits. Unscoped: at the limit.
- `Sync` with a stub store: err=nil, 1532 parents = 1532 regular non-index `.md`.
The prefix check does not suppress legitimate scoped search.
### Flip tests: all red
| Mutation | Red |
|---|---|
| Check only the full path (old behaviour) | both new tests |
| Check only the first component | both search wing tests |
| `Lstat` → `Stat` in realDir | 5 tests across brainfs, search and vectorstore |
| IsDir check dropped | the same 5 |
| `rel == "."`-style bypass widened to `wiki*` | both search wing tests |
The round-1 and round-2 flips are unchanged and were red at 66b6cf8. sync.go has not changed since.
**Rule 9:** `TestWalkRegularReportsASymlinkedIntermediateComponentOfTheWalkRoot`, `TestQueryDoesNotFollowASymlinkedWingRootInsideTheBrain` and `TestQueryDoesNotFollowASymlinkedWingUnderAWingAndHallRoot` match their assertions.
### Doc wording on the residual: accurate
`realDir` says "a swap after the check is not caught". `WalkRegular` says a directory swapped between listing and read is followed, and that os.Root keeps the read inside the brain but not inside relDir. Both match what I measured:
- Root swap after the check (`knowledge` ↔ symlink to `sessions`): 9 of about 202k Syncs embedded session text.
- Subdir swap (`wiki/w` ↔ `../sessions`): 10,597 of about 206k.
- 0 outside-brain leaks in every race.
That is the residual exactly as stated. Accepting it or redesigning (an fd-relative walk) is Mathias's decision. It is not part of this verdict.
### Gate and CI
- CI run 9060 on 31071bc: success.
- Local `CI=1 task check`: EXIT=0, 0 FAILs. The worktree is clean.
- `-race -count=10` on vectorstore, brainfs and search: green.
### Non-blocking
- The two new search tests only assert empty results, with no in-test positive control. The flips going red, and the real-brain scoped query returning 27/27, show the path is live.
- The error text "walk root component X is not a directory: not a regular file" reads oddly. An `ErrNotDir` sentinel would read better.
### Not checked
- Real device nodes (they need root).
- A real Syncthing peer.
- CI job logs (commit status only).
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 3 of the reader migration). risk/medium: control code, adversarial review required (rule 8).
Problem
vectorstore.Syncwalkedwiki/andknowledge/withfilepath.WalkDirand read each.mdwithos.ReadFile. A symlinked note (Syncthing syncs symlinks into/var/lib/supervisor/brain) had its target embedded into pgvector and served by semantic search. A FIFO stalled the sync.Change
brainfs.WalkRegular, read viabrainfs.ReadRegular: regular files only, type checked on the opened handle.brainfs.Lstatadded for the mtime staleness check.Live effect on deploy
Chunks already embedded from a symlinked note are deleted on the first sync, because the note is no longer seen. That is the intended purge.
Tests and flips
TestSyncRefusesSymlinksAndSurvivesSwapRaces: static symlink plus a positive control, and brainfstest leak/hang races. Red before the fix (static leak, 139k leaks in the race).TestSyncDeletesChunksOfASymlinkedNoteTestSyncAbortsBeforeDeletingWhenASubdirIsUnreadableTestLstatDoesNotFollowSymlinksAndRefusesEscapesFlips:
os.ReadFileinstead ofReadRegular→ redos.Statinstead ofbfs.Lstat→ stays green, by design: the walk has already filtered to regular files, so the stat only decides re-embed timing, and the read guard is the control.Not covered
🤖 Generated with Claude Code
Created via git-mcp on behalf of @mathias
pr-gatekeeper verdict: BLOCK (PR #166 @
74a9621). Not merged.CI run 9058 (
CI / Lint / Test / Vet) is green. Locally,CI=1 task checkgave EXIT=0 with 0 FAILs, and-race -count=10on vectorstore and brainfs is green. None of that covers the finding below.Blocking
ingestion/internal/vectorstore/sync.go(thebfs.WalkRegular(sub, ...)call) together withbrainfs.WalkRegular: a symlinked scan root is now followed.maindid not follow it.fs.WalkDirstats its root withfs.Stat, which follows symlinks.os.Rootallows that as long as the target stays inside the brain. So ifknowledgeorwikiis a symlink to somewhere else in the brain, Sync walks the target.filepath.WalkDironmainLstat'ed the root. It saw a non-directory and did not descend.knowledge -> ., Sync now embeds every.mdin the brain under aknowledge/alias:sessions/(in.stignore, so koala-local),raw/(the inbox), and root files such asCLAUDE.md.hydratereadsknowledge/sessions/x.mdthroughReadRegular, which follows the in-root intermediate symlink.Repro (executed). Brain contents:
wiki/w/a.md,sessions/s1.md,raw/r1.mdandCLAUDE.md, each with its own sentinel, plusknowledge -> .. Thenvectorstore.Syncwith a recording embedder:74a9621knowledge/CLAUDE.md,knowledge/raw/r1.md,knowledge/sessions/s1.md,knowledge/wiki/w/a.md,wiki/w/a.mdorigin/mainsync.gowiki/w/a.mdmainin the reader this PR hardens, and it contradictsWalkRegular's doc ("symlinks ... are skipped, never followed").Lstatthe scan root through the root and require a real directory. If it is missing, skip as today. If it is a symlink or not a directory, either skip it or return an error. Then pin it with a test that has a symlinkedknowledgepointing inside the brain and asserts nothing outsideknowledge/is embedded. Doing this inWalkRegularinstead also fixes the other readers.Non-blocking
rel == subrestriction inonErris unpinned. Flip: changerel == sub && errors.Is(err, fs.ErrNotExist)toerrors.Is(err, fs.ErrNotExist). The suite stays green. The PR body says "onErr swallowing every error → red", but swallowing every not-exist error is not covered. Effect: a subdir that vanishes mid-walk (Syncthing delete or rename) would be skipped and its chunks deleted, instead of aborting the run. In practice that means one tick of re-embed churn. Add a test.TestSyncRefusesSymlinksAndSurvivesSwapRaceshas no positive control of its own. Itssyncclosure discards the error (_, _ = vectorstore.Sync), andAssertNoLeakdoes not require that the inside note was ever read. The first half's positive control and the red no-handle-check and no-O_NONBLOCK flips show the race is exercised today, so this is not decoration now. It would become decoration if Sync started erroring early under the swap.wiki/knowledgesymlink aborts every Sync (statat wiki: path escapes from parent).knowledge/is then never synced either. This fails closed and is logged, and the tripwire heals it. Recording the behaviour so it is not a surprise.wikisymlink deletes everywiki/chunk (ErrNotExiston the root, which is skipped). Same behaviour asmain, so not a regression.risk/high. The PR body says risk/medium and the PR has no labels. The label decides the lane.hydratereads vector-hit paths withReadRegular, which follows in-root intermediate symlinks. Even after this fix, a stale alias row resolves while the link exists.Probes run (by execution, scratch copy of the module)
wiki -> outside dirwiki -> other(relative, inside)wiki -> /abs/brain/otherwiki/w -> outsidewiki/w/*chunks deletedwiki/w -> ../knowledge-> /dev/zero,-> /dev/nullknowledge -> .wikiswapped between a real dir and a symlink to outside, about 127k Syncsknowledge/chunkswiki/w(intermediate) swapped between a dir and a symlink to outside, about 116k Syncs.mdon disk; 0 item errorsLive layout check:
wikiandknowledgeunder/var/lib/supervisor/brainare real directories with 0 symlinks and 0 non-regular entries. The deploy itself would not break.Flip tests
os.ReadFileinstead ofReadRegularseenParentsset only after a successful readWalkRegulardescends into or yields symlinksTestSyncDeletesChunksOfASymlinkedNote)O_NONBLOCKremovedbrainfs.Lstat→root.StatRule 9:
TestSyncDeletesChunksOfASymlinkedNote,TestSyncAbortsBeforeDeletingWhenASubdirIsUnreadableandTestLstatDoesNotFollowSymlinksAndRefusesEscapesmatch their assertions.TestSyncRefusesSymlinksAndSurvivesSwapRacescovers only a symlinked file. With the blocking finding open, "RefusesSymlinks" claims more than the code does for a symlinked scan root. Either narrow the name or add the root case.Not checked
DT_CHRskip and the handle check are covered by reading the code and by the FIFO race.pr-gatekeeper verdict, round 2: BLOCK (PR #166 @
66b6cf8). Not merged.The round-1 blocker is closed for
vectorstore.Sync, verified by execution. Search's new root check misses one case, shown below.CI=1 task checkgave EXIT=0 with 0 FAILs, and the worktree is clean.-race -count=10on vectorstore, brainfs and search is green.Blocking
ingestion/internal/brainfs/brainfs.goWalkRegular: the root check Lstats only the last component ofrelDir. A symlink in an earlier component is still followed, so search with Wing+Hall walks an aliased directory.os.Root.Lstat("wiki/alias/facts")followsalias. The final componentfactsis a real directory, so the check passes andfs.WalkDirwalks the alias target.raw/facts/r.mdcontainingzqrawhall, pluswiki/alias -> ../raw.Query(Query:"zqrawhall", Wing:"alias", Hall:"facts")→ err=nil, hits=1.TestQueryDoesNotFollowASymlinkedSearchRootInsideTheBraincovers only a single-component wing root. Rule 9: the name claims the search root, and with a hall the root iswiki/<wing>/<hall>.sessions/) have none, so in practice it aliases content a peer could already see. But the fix is a correction:WalkRegular, Lstat every prefix ofrelDir(wiki,wiki/alias,wiki/alias/facts) and require each to be a real directory.Escalate to Mathias (accept or fix; this is a judgement call, not a correction)
A directory swapped for an in-brain symlink between listing and reading is followed. This applies at every depth, and also around the new root check.
os.Rootkeeps reads inside the brain, but not inside the scan subtree.Executed: about 210k Syncs per probe while a swapper toggles a path between a real dir and a symlink. Session file
sessions/s1.mdcarries a sentinel.knowledge(scan root, TOCTOU on the new Lstat).knowledgesessionswiki/w(subdir)../sessionsfs.WalkDirseeswas a dir, then callsReadDir("wiki/w")by path, which follows a swapped-in symlink.ReadRegular("wiki/w/s1.md")follows the intermediate the same way.main(filepath.WalkDir + os.ReadFile) has the same race and the static file-symlink follow besides.sessions/. pgvector stores vectors, not text. The tripwire removes the link within 30 s.os.SameFileagainst the Lstat result, and read relative to that handle. That is not a one-line correction.WalkRegular's doc ("symlinks ... never followed") overstates the guarantee under a race. Either qualify it, or record this as an accepted residual on #157 next to the hard-link one.Round-1 items: verified
knowledge -> .knowledge/upserts; 0 deleteswikipointing outside, absolute-inside, relative-inside, dangling absolute or dangling relativewikiused to delete every wiki chunk, so that is an improvement/dev/*wikiabout 438k Syncs,wiki/wabout 243k).mdrisk/highFlip tests
These went red:
WalkRegularrootLstat→Stat: brainfs, vectorstore and search tests red.IsDircheck removed: all three red.TestSyncAbortsOnASymlinkedScanRootred.TestSyncSkipsAMissingScanRoot....SubdirIsUnreadableand...SymlinkedScanRootred.None of my flips survived. The gap above is a case no test exercises, not a flip that stays green.
Rule 9: the brainfs and vectorstore names now match their assertions;
TestSyncRefusesSymlinkedNotesAndSurvivesSwapRacesis honest. The search name is addressed above. The missing per-iteration positive control on the race half is not blocking: the handle-check and O_NONBLOCK flips show the race is live.Cosmetic
The root error reads "walk root is not a directory: not a regular file", because
ErrNotRegularis wrapped for a directory check. A sentinel such asErrNotDirwould read better.Not checked
WalkRegularcallers besides Sync and search: none are in this diff.pr-gatekeeper verdict, round 3: PASS (PR #166 @
31071bc). Not merged.This is a verdict only. The PR is
risk/highand CD deploys on merge, so the merge is Mathias's call. The race residual is going to him separately; see the end.Round-2 blocker: closed, verified by execution
realDirLstats every prefix of the walk root. There are twoWalkRegularcallers (found by grep, non-test):search.Queryandvectorstore.Sync. Both are covered below.wiki/alias -> ../raw, Wing=alias, Hall=facts (my round-2 repro)wiki/real/facts -> ../../rawwiki -> raw: unscoped query, and Wing=facts under it../raw(traversal)knowledge -> .;wikipointing outside, absolute-inside, relative-inside or dangling/dev/*wikiabout 409k Syncs,wiki/wabout 230k)Positive controls on the real brain (read-only, counts only):
Query, Wing+Hall on a real wing'sfacts: 27 hits, equal to the 27 matching files on disk.Syncwith a stub store: err=nil, 1532 parents = 1532 regular non-index.md.The prefix check does not suppress legitimate scoped search.
Flip tests: all red
Lstat→Statin realDirrel == "."-style bypass widened towiki*The round-1 and round-2 flips are unchanged and were red at
66b6cf8. sync.go has not changed since.Rule 9:
TestWalkRegularReportsASymlinkedIntermediateComponentOfTheWalkRoot,TestQueryDoesNotFollowASymlinkedWingRootInsideTheBrainandTestQueryDoesNotFollowASymlinkedWingUnderAWingAndHallRootmatch their assertions.Doc wording on the residual: accurate
realDirsays "a swap after the check is not caught".WalkRegularsays a directory swapped between listing and read is followed, and that os.Root keeps the read inside the brain but not inside relDir. Both match what I measured:knowledge↔ symlink tosessions): 9 of about 202k Syncs embedded session text.wiki/w↔../sessions): 10,597 of about 206k.That is the residual exactly as stated. Accepting it or redesigning (an fd-relative walk) is Mathias's decision. It is not part of this verdict.
Gate and CI
31071bc: success.CI=1 task check: EXIT=0, 0 FAILs. The worktree is clean.-race -count=10on vectorstore, brainfs and search: green.Non-blocking
ErrNotDirsentinel would read better.Not checked