#157 slices 1–3: per-caller tests with in-brain symlink targets (search, hydrate, vectorstore, context excerpt, ReadNote) #171

New Issue

2026-10-04T20:41:05Z

mathias commented

2026-10-04 20:41:05 +00:00

Source: PR #168 round-2 review (PASS), 2026-10-04.

Gap

The merged slice 1–3 readers have swap and symlink tests whose targets are only outside the brain, and os.Root refuses those on its own. The reviewer made each caller read without the Lstat or SameFile check, and every one stayed green:

search.Query (search.go:146)
hydrate (search.go:297)
vectorstore.Sync (sync.go:145)
the context excerpt (tools_context.go:190)
api.ReadNote (update.go:188)

The code is safe today, because they all go through brainfs.ReadRegular, and PR #168 added the SameFile check there, unit-tested in brainfs. But no per-caller test would catch a caller that stopped using it. That is the gap class the #168 review blocked on.

Fix

For each caller, next to every outside-target case:

a static in-brain link (e.g. wiki/w/facts/inlink.md -> ../../../raw/s.md)
brainfstest.AssertNoLeakFrom with an in-brain target (brainfstest.WriteSentinelAt)

Flip-test each caller with a bypass of the Lstat or SameFile check.

Also from that review (non-blocking)

Rule 9 nit: rename TestReadRegularNeverReturnsAnInBrainFileSwappedInBehindASymlink, since "Never" overclaims a sampled race. "DoesNotReturn" would be honest.
Cosmetic: AssertNoLeakFrom's message says "outside file" even for in-brain targets.
SameFile side effect (accepted, recorded here): it also refuses a legitimate replace-by-rename (how Syncthing writes) that lands between the Lstat and the open. Every caller heals on its next run: the inventory falls back to the slug, the index summary is empty, IndexDoc logs a warning, the backfill aborts that root, and vectorstore re-embeds at the next sync.

Created via git-mcp on behalf of @mathias

**Source:** PR #168 round-2 review (PASS), 2026-10-04. ## Gap The merged slice 1–3 readers have swap and symlink tests whose targets are only **outside** the brain, and os.Root refuses those on its own. The reviewer made each caller read without the Lstat or SameFile check, and **every one stayed green**: - `search.Query` (search.go:146) - `hydrate` (search.go:297) - `vectorstore.Sync` (sync.go:145) - the context excerpt (tools_context.go:190) - `api.ReadNote` (update.go:188) The code is safe today, because they all go through `brainfs.ReadRegular`, and PR #168 added the SameFile check there, unit-tested in brainfs. But no per-caller test would catch a caller that stopped using it. That is the gap class the #168 review blocked on. ## Fix For each caller, next to every outside-target case: - a static in-brain link (e.g. `wiki/w/facts/inlink.md -> ../../../raw/s.md`) - `brainfstest.AssertNoLeakFrom` with an in-brain target (`brainfstest.WriteSentinelAt`) Flip-test each caller with a bypass of the Lstat or SameFile check. ## Also from that review (non-blocking) - **Rule 9 nit:** rename `TestReadRegularNeverReturnsAnInBrainFileSwappedInBehindASymlink`, since "Never" overclaims a sampled race. "DoesNotReturn" would be honest. - **Cosmetic:** `AssertNoLeakFrom`'s message says "outside file" even for in-brain targets. - **SameFile side effect (accepted, recorded here):** it also refuses a legitimate replace-by-rename (how Syncthing writes) that lands between the Lstat and the open. Every caller heals on its next run: the inventory falls back to the slug, the index summary is empty, IndexDoc logs a warning, the backfill aborts that root, and vectorstore re-embeds at the next sync. --- _Created via git-mcp on behalf of @mathias_

mathias added the risk/high label 2026-10-04 20:41:05 +00:00

mathias referenced this issue from a commit

2026-10-05 05:02:42 +00:00

test(brainfs): per-caller in-brain symlink targets for slices 1-3 (#171)

mathias referenced a pull request that will close this issue

2026-10-05 05:02:50 +00:00

test(brainfs): per-caller in-brain symlink targets for slices 1-3 (#171) #172

mathias referenced this issue

2026-10-05 05:04:24 +00:00

SECURITY: symlinks anywhere in the Syncthing-shared brain dir are followed by search, vectorstore, context and graph readers #157

mathias referenced this issue

2026-10-05 05:17:32 +00:00

test(brainfs): per-caller in-brain symlink targets for slices 1-3 (#171) #172

mathias commented

2026-10-05 05:17:32 +00:00

pr-gatekeeper verdict: BLOCK (PR #172 @ `bf3646c`). Not merged.

The test additions do what the PR claims, and I verified that by running them. One rule-9 finding blocks. Separately, the PR is risk/high, so even after a PASS the merge is Mathias's call.

Blocking: rule 9

search/brainroot_test.go: TestQueryLeaksNothingFromAnInBrainTargetUnderASymlinkSwapRace and TestHydrateRefusesAnInBrainSymlinkAndLeaksNothingUnderASwapRace.

"LeaksNothing" makes the same universal claim over a sampled 1.5 s race as the "Never" that this PR renames, following #171's own nit.
The assertion proves 0 leaks in the sampled iterations, nothing more.
Fix: rename to match TestReadRegularDoesNotReturnAnInBrainFileSwappedInBehindASymlink, e.g. ...DoesNotReturnAnInBrainTarget....
This is a correction, not a judgement call.

Flip tests (executed, 5 callers x 3 bypasses)

Bypass at the caller's ReadRegular site	Pre-PR tests	PR tests
`os.OpenRoot(brainDir).ReadFile` (the PR's claim)	red only via the existing FIFO-hang test	red 5/5 per caller, on the new in-brain assertions (Query, hydrate) or in the edited tests (Sync, excerpt, ReadNote)
Lstat + ReadFile (hydrate, excerpt, ReadNote)	not run	red 5/5 each
Lstat + non-blocking open + handle type check, with no SameFile (the gap #171 describes)	green on all 5	red 3/3 on all 5

The last row is the decisive one. The SameFile-only bypass that #171 says nothing caught is now caught per caller, and before this PR it was not.

Stability, gate, CI

go test -race -count=10 on search, vectorstore, mcp, api and brainfs: EXIT=0, 0 FAIL, 0 DATA RACE.
CI=1 task check at bf3646c: EXIT=0, 0 FAIL. The run included the four ingestion packages and gitleaks.
CI run 9296 on bf3646c: success.
Main has moved to b6ef116 (two commits, cmd/server and go.mod, no overlap). A local merge is clean, and the affected packages plus cmd/server pass on the merged tree. No rebase is needed.

Done-when (#171)

All met, apart from the blocking item above:

a static in-brain link per caller
AssertNoLeakFrom with an in-brain target per caller
flips executed
the rename
the failure message

Non-blocking

Query and Sync: the static in-brain link case is masked by WalkRegular, so only the race catches a caller bypass there. The PR body already says so.
The Query swap test has no positive control. Its search term never matches "inside", and rs, _ := Query(...) discards errors. The flip proves it observes a leak, but a Query that always errors would still pass it.
The brainfstest package doc still says "the outside file" for what can now be an in-brain target.

Not checked

The live deploy, because this change is test-only.
Hard links, and the directory-swap race (#167, accepted).
Callers outside slices 1-3.

## pr-gatekeeper verdict: **BLOCK** (PR #172 @ bf3646c). Not merged. The test additions do what the PR claims, and I verified that by running them. One rule-9 finding blocks. Separately, the PR is `risk/high`, so even after a PASS the merge is Mathias's call. ### Blocking: rule 9 `search/brainroot_test.go`: `TestQueryLeaksNothingFromAnInBrainTargetUnderASymlinkSwapRace` and `TestHydrateRefusesAnInBrainSymlinkAndLeaksNothingUnderASwapRace`. - "LeaksNothing" makes the same universal claim over a sampled 1.5 s race as the "Never" that this PR renames, following #171's own nit. - The assertion proves 0 leaks in the sampled iterations, nothing more. - Fix: rename to match `TestReadRegularDoesNotReturnAnInBrainFileSwappedInBehindASymlink`, e.g. `...DoesNotReturnAnInBrainTarget...`. - This is a correction, not a judgement call. ### Flip tests (executed, 5 callers x 3 bypasses) | Bypass at the caller's ReadRegular site | Pre-PR tests | PR tests | |---|---|---| | `os.OpenRoot(brainDir).ReadFile` (the PR's claim) | red only via the existing FIFO-hang test | red 5/5 per caller, on the new in-brain assertions (Query, hydrate) or in the edited tests (Sync, excerpt, ReadNote) | | Lstat + ReadFile (hydrate, excerpt, ReadNote) | not run | red 5/5 each | | **Lstat + non-blocking open + handle type check, with no SameFile** (the gap #171 describes) | **green on all 5** | **red 3/3 on all 5** | The last row is the decisive one. The SameFile-only bypass that #171 says nothing caught is now caught per caller, and before this PR it was not. ### Stability, gate, CI - `go test -race -count=10` on search, vectorstore, mcp, api and brainfs: EXIT=0, 0 FAIL, 0 DATA RACE. - `CI=1 task check` at bf3646c: EXIT=0, 0 FAIL. The run included the four ingestion packages and gitleaks. - CI run 9296 on bf3646c: success. - Main has moved to b6ef116 (two commits, cmd/server and go.mod, no overlap). A local merge is clean, and the affected packages plus cmd/server pass on the merged tree. No rebase is needed. ### Done-when (#171) All met, apart from the blocking item above: - a static in-brain link per caller - `AssertNoLeakFrom` with an in-brain target per caller - flips executed - the rename - the failure message ### Non-blocking - Query and Sync: the static in-brain link case is masked by WalkRegular, so only the race catches a caller bypass there. The PR body already says so. - The Query swap test has no positive control. Its search term never matches "inside", and `rs, _ := Query(...)` discards errors. The flip proves it observes a leak, but a Query that always errors would still pass it. - The `brainfstest` package doc still says "the outside file" for what can now be an in-brain target. ### Not checked - The live deploy, because this change is test-only. - Hard links, and the directory-swap race (#167, accepted). - Callers outside slices 1-3.

mathias referenced this issue

2026-10-05 05:18:18 +00:00

test(brainfs): per-caller in-brain symlink targets for slices 1-3 (#171) #172

mathias commented

2026-10-05 05:18:19 +00:00

pr-gatekeeper verdict on PR #172 @ bf3646c: BLOCK. notes.ReadScoped (internal/notes/edit.go:83, a slice-2 caller missing from the list above) stays green under a SameFile bypass, and two new test names overclaim (rule 9). Evidence is in #172 (comment)

pr-gatekeeper verdict on PR #172 @ bf3646c: **BLOCK**. `notes.ReadScoped` (internal/notes/edit.go:83, a slice-2 caller missing from the list above) stays green under a SameFile bypass, and two new test names overclaim (rule 9). Evidence is in https://git.d-ma.be/mathias/hyperguild/pulls/172#issuecomment-15358

mathias referenced this issue from a commit

2026-10-05 05:19:29 +00:00

test(search): name the in-brain swap-race tests after what they sample

mathias referenced this issue

2026-10-05 05:21:36 +00:00

fix(pipeline,classification): read the brain through brainfs (#157, slice 6) #176

mathias referenced this issue

2026-10-05 05:21:37 +00:00

SECURITY: symlinks anywhere in the Syncthing-shared brain dir are followed by search, vectorstore, context and graph readers #157

mathias commented

2026-10-05 05:25:56 +00:00

pr-gatekeeper re-review of PR #172 @ f969d97: PASS (not merged, risk/high). Findings 1 and 2 are resolved, and the ReadScoped flips went red 18/18. Evidence is on the PR.

pr-gatekeeper re-review of PR #172 @ f969d97: **PASS** (not merged, risk/high). Findings 1 and 2 are resolved, and the ReadScoped flips went red 18/18. Evidence is on the PR.

mathias referenced this issue

2026-10-05 18:58:23 +00:00

SECURITY: symlinks anywhere in the Syncthing-shared brain dir are followed by search, vectorstore, context and graph readers #157

mathias referenced this issue

2026-10-05 19:04:51 +00:00

fix(brainfs): refuse a file behind a symlinked directory for reads and writes (#173) #187

mathias referenced this issue

2026-10-05 19:13:05 +00:00

fix(brainfs): refuse a file behind a symlinked directory for reads and writes (#173) #187

mathias closed this issue

2026-10-05 19:36:31 +00:00

mathias referenced this issue from a commit