Closes#171. Test-only (plus a brainfstest failure message). risk/high per the issue label, so it goes through a PR.
Gap: os.Root refuses links that point outside the brain, so the outside-target tests for slices 1–3 stayed green when a caller skipped brainfs's Lstat/SameFile checks.
Added, per caller (search.Query, hydrate, vectorstore.Sync, readExcerpt, api.ReadNote):
a static in-brain link, wiki/w/facts/inlink.md -> ../../../raw/s.md
brainfstest.AssertNoLeakFrom with an in-brain target
Flip evidence (executed): I replaced each caller's bfs.ReadRegular(x) with os.OpenRoot(brainDir).ReadFile(x). Every caller went red, each on the new assertions:
Closes #171. Test-only (plus a brainfstest failure message). risk/high per the issue label, so it goes through a PR.
**Gap:** os.Root refuses links that point outside the brain, so the outside-target tests for slices 1–3 stayed green when a caller skipped brainfs's Lstat/SameFile checks.
**Added, per caller (search.Query, hydrate, vectorstore.Sync, readExcerpt, api.ReadNote):**
- a static in-brain link, `wiki/w/facts/inlink.md -> ../../../raw/s.md`
- `brainfstest.AssertNoLeakFrom` with an in-brain target
**Flip evidence (executed):** I replaced each caller's `bfs.ReadRegular(x)` with `os.OpenRoot(brainDir).ReadFile(x)`. Every caller went red, each on the new assertions:
- Query: `TestQueryLeaksNothingFromAnInBrainTargetUnderASymlinkSwapRace`
- hydrate: `TestHydrateRefusesAnInBrainSymlink...`
- Sync: in-brain target leaked 22,270 times
- readExcerpt: static link (line 28) red, and 131,070 swap leaks
- ReadNote: static link (lines 31–32) red, and 126,920 swap leaks
Under Sync and Query, WalkRegular already skips a static link, so only the swap race catches those two bypasses.
**Also:**
- Rule 9 rename: `TestReadRegularDoesNotReturnAnInBrainFileSwappedInBehindASymlink`
- `AssertNoLeakFrom`'s message now names the target
**Gate:** `CI=1 task check` EXIT=0, 0 FAILs.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---
_Created via git-mcp on behalf of @mathias_
os.Root refuses links that leave the brain on its own, so the slice 1-3
callers' outside-target tests stayed green when a caller read without
brainfs's Lstat/SameFile checks (PR #168 round-2 review). Adds a static
in-brain link (wiki/w/facts/inlink.md -> ../../../raw/s.md) and an
AssertNoLeakFrom swap race with an in-brain target for search.Query,
hydrate, vectorstore.Sync, readExcerpt and api.ReadNote.
Flip-verified: replacing each caller's bfs.ReadRegular with an
os.OpenRoot(brainDir).ReadFile turns every one red on the new assertions
(Query, hydrate, Sync, readExcerpt, ReadNote).
Also from that review: rename TestReadRegularNever... to
...DoesNotReturn... (rule 9: a sampled race), and AssertNoLeakFrom's
failure message names the symlink target instead of 'outside file'.
Closes#171
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
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.
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.
Mode: PR. Reviewed by executing flips in a fresh clone (~/.cache/hg-work/r172). I did not merge or close anything.
Findings (most severe first)
internal/notes/edit.go:83: notes.ReadScoped (brain_read, slice 2, #165) is a slice 1–3 caller that still has no in-brain target. This is the gap class #171 exists to close. I bypassed the SameFile check there (Lstat, then the handle check, then read with no SameFile) and TestReadScopedRefusesSymlinksAndSurvivesSwapRaces stayed green 3/3. With the Lstat dropped as well, it was also green 3/3. internal/notes/read_root_test.go only calls AssertNoLeak, whose target is outside the brain, and os.Root refuses that on its own. #171's list of five missed this sixth caller, and the PR copies that list. So the PR claim "each slice 1–3 caller" is false for 1 of 6. Fix: add brainfstest.AssertNoLeakFrom(t, p, "inside", brainfstest.WriteSentinelAt(t, filepath.Join(brain, "raw", "s.md")), …, read) to that test, then flip it. Slice 4 (graphsync.go:71, wiki/inventory.go:55, wiki/index.go:60) is already covered: all three went red under both bypasses.
Rule 9:TestQueryLeaksNothingFromAnInBrainTargetUnderASymlinkSwapRace and TestHydrateRefuses…AndLeaksNothingUnderASwapRace. "LeaksNothing" is a universal claim about a race that is sampled for 1.5 s. That is the same overclaim #171 asked to remove from "Never", and this PR renames that one while adding two new ones. Honest names would be …DoesNotLeakAnInBrainTarget… / …AndDoesNotLeakItUnderASwapRace.
(non-blocking) search/brainroot_test.go:186-194: the Query race reader discards Query's error, and the test has no positive control. I pointed brainfs.Open at a nonexistent dir so that every Query errors, and the new test stayed green (11 other tests in the package went red). Hydrate's test has the positive control and Query's does not.
(non-blocking, pre-existing) AssertNoLeakFrom has no minimum for iterations or for successful reads. If the swapper's os.Symlink fails, which it does silently because of _ =, the test passes without proving anything.
Variants: os.OpenRoot(dir).ReadFile · fs.ReadFile(root.FS()) · Lstat-only + root.ReadFile · Lstat + handle check with no SameFile · os.ReadFile after Lstat · handle check only with no Lstat.
PR tests: 30/30 red. Each flip went red on the new assertion or test.
Same flips against origin/main's tests: hydrate was green 6/6. With no SameFile, and with no Lstat, Query, Sync, excerpt and ReadNote were all green. So the new assertions are what catch these flips.
No SameFile, -count=5 per site: 5/5 red at each site, with thousands of leaks per run. For example Sync leaked 4.6k–7.5k times in about 45k iterations, and ReadNote about 77k times in about 260k.
The static-link asserts fire under the no-Lstat variant (hydrate :208-209, excerpt :28, ReadNote :31-32). Query's static assert (:199) fires only once WalkRegular also yields symlinks, which matches what the PR body says.
Unflipped -count=5 on search, vectorstore, mcp, api and brainfs: all ok, no flakes.
Gate / CI
CI=1 GOFLAGS=-p=2 task check at bf3646c: EXIT=0, 0 FAIL lines, and the log shows the ingestion packages ran.
CI run 9296 (pull_request, bf3646c): success. Mirror job skipped. Mergeable.
CI is green, and that green covers neither finding 1 nor finding 2.
Not checked
Hard-link targets (an accepted residual in brainfs).
Callers outside the brainfs set.
Whether Resolve's own tests (path_test.go) use an in-brain static target.
Behaviour under heavier CI load than -p=2.
## pr-gatekeeper verdict: BLOCK (hyperguild#172 @ bf3646c)
Mode: PR. Reviewed by executing flips in a fresh clone (`~/.cache/hg-work/r172`). I did not merge or close anything.
### Findings (most severe first)
1. **`internal/notes/edit.go:83`: `notes.ReadScoped` (brain_read, slice 2, #165) is a slice 1–3 caller that still has no in-brain target.** This is the gap class #171 exists to close. I bypassed the SameFile check there (Lstat, then the handle check, then read with no SameFile) and `TestReadScopedRefusesSymlinksAndSurvivesSwapRaces` stayed **green 3/3**. With the Lstat dropped as well, it was also **green 3/3**. `internal/notes/read_root_test.go` only calls `AssertNoLeak`, whose target is outside the brain, and os.Root refuses that on its own. #171's list of five missed this sixth caller, and the PR copies that list. So the PR claim "each slice 1–3 caller" is false for 1 of 6. **Fix:** add `brainfstest.AssertNoLeakFrom(t, p, "inside", brainfstest.WriteSentinelAt(t, filepath.Join(brain, "raw", "s.md")), …, read)` to that test, then flip it. Slice 4 (`graphsync.go:71`, `wiki/inventory.go:55`, `wiki/index.go:60`) is already covered: all three went red under both bypasses.
2. **Rule 9:** `TestQueryLeaksNothingFromAnInBrainTargetUnderASymlinkSwapRace` and `TestHydrateRefuses…AndLeaksNothingUnderASwapRace`. "LeaksNothing" is a universal claim about a race that is sampled for 1.5 s. That is the same overclaim #171 asked to remove from "Never", and this PR renames that one while adding two new ones. Honest names would be `…DoesNotLeakAnInBrainTarget…` / `…AndDoesNotLeakItUnderASwapRace`.
3. (non-blocking) `search/brainroot_test.go:186-194`: the Query race reader discards Query's error, and the test has no positive control. I pointed `brainfs.Open` at a nonexistent dir so that every Query errors, and the new test stayed **green** (11 other tests in the package went red). Hydrate's test has the positive control and Query's does not.
4. (non-blocking, pre-existing) `AssertNoLeakFrom` has no minimum for iterations or for successful reads. If the swapper's `os.Symlink` fails, which it does silently because of `_ =`, the test passes without proving anything.
### Flip tests (executed: 5 listed sites × 6 bypass variants)
Variants: `os.OpenRoot(dir).ReadFile` · `fs.ReadFile(root.FS())` · Lstat-only + root.ReadFile · Lstat + handle check with no SameFile · `os.ReadFile` after Lstat · handle check only with no Lstat.
- **PR tests: 30/30 red.** Each flip went red on the new assertion or test.
- **Same flips against origin/main's tests:** hydrate was green 6/6. With no SameFile, and with no Lstat, Query, Sync, excerpt and ReadNote were all green. So the new assertions are what catch these flips.
- No SameFile, `-count=5` per site: 5/5 red at each site, with thousands of leaks per run. For example Sync leaked 4.6k–7.5k times in about 45k iterations, and ReadNote about 77k times in about 260k.
- The static-link asserts fire under the no-Lstat variant (hydrate :208-209, excerpt :28, ReadNote :31-32). Query's static assert (:199) fires only once WalkRegular also yields symlinks, which matches what the PR body says.
- Unflipped `-count=5` on search, vectorstore, mcp, api and brainfs: all ok, no flakes.
### Gate / CI
- `CI=1 GOFLAGS=-p=2 task check` at bf3646c: **EXIT=0**, 0 FAIL lines, and the log shows the ingestion packages ran.
- CI run **9296** (pull_request, bf3646c): success. Mirror job skipped. Mergeable.
- CI is green, and that green covers neither finding 1 nor finding 2.
### Not checked
- Hard-link targets (an accepted residual in brainfs).
- Callers outside the brainfs set.
- Whether `Resolve`'s own tests (`path_test.go`) use an in-brain static target.
- Behaviour under heavier CI load than `-p=2`.
Review of #172 (rule 9): "LeaksNothing" claims more than a 1.5s
sampled race can prove -- the same overclaim this PR removes from the
"Never" names. Renamed to DoesNotReturn...DuringASampledSwapRace,
and the brainfstest doc now says the target may be inside the brain
(#171) and that observation is over a sampled window.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of #172 (finding 1): notes.ReadScoped (brain_read, slice 2)
had only outside-brain targets, which os.Root refuses on its own, so a
reader bypassing brainfs's Lstat/SameFile stayed green. Adds a static
in-brain link, a positive control and an in-brain swap race.
Flip: replacing ReadRegularMax with os.OpenRoot().ReadFile turns it
red 3/3.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pr-gatekeeper re-review: PASS (hyperguild#172 @ f969d97). Not merged: risk/high, so Mathias merges.
Mode: PR. Re-review of a83f0cb + f969d97 on top of bf3646c (BLOCK, comment 15358). Flips were executed in a fresh clone.
Finding 1 (ReadScoped, internal/notes/edit.go:83): resolved. I ran all 6 bypass variants (os.OpenRoot().ReadFile, fs.ReadFile(root.FS()), Lstat-only + root.ReadFile, Lstat + handle check with no SameFile, os.ReadFile after Lstat, handle check only). Each ran 3 times, and TestReadScopedDoesNotReturnAnInBrainSymlinkTargetStaticallyOrDuringASampledSwapRace went red 18/18, at read_root_test.go:67 (the AssertNoLeakFrom race). For the two narrowest variants I preserved the size limit, so the only failure is the new test, without the over-limit size test also failing just because the bypass dropped the limit: red 3/3 each. No SameFile leaked about 5k times in about 515k iterations; no Lstat leaked about 265k times in about 420k. Before this commit both variants were green 3/3.
Note: the static assertions (:52-53) never fired under any variant. That is because Resolve's Lstat refuses a static link before the read, so they pin ReadScoped's behaviour, not edit.go:83. The test name describes the behaviour, so this is accurate.
Finding 2 (rule 9): resolved.TestQueryDoesNotReturnAnInBrainSymlinkTargetDuringASampledSwapRace, TestHydrateRefusesAnInBrainSymlinkAndDoesNotReturnItsTargetDuringASampledSwapRace and the ReadScoped name each claim no more than their assertions prove. The brainfstest doc now says "sampled window only".
Findings 3 and 4 (non-blocking): the coordinator is filing these as a follow-up issue.
Flakiness: unflipped -count=5 on notes and search: ok. Gate:CI=1 GOFLAGS=-p=2 task check at f969d97 gave EXIT=0 and 0 FAIL lines, and the notes package ran. CI: run 9330 on f969d97 succeeded (mirror job skipped). Mergeable. That green adds nothing beyond the flips above.
Not checked: hard-link targets, read paths outside brainfs, and pre-existing names on main (for example TestQueryLeaksNothingUnderASymlinkSwapRace), which are outside this PR's diff.
## pr-gatekeeper re-review: PASS (hyperguild#172 @ f969d97). Not merged: risk/high, so Mathias merges.
Mode: PR. Re-review of a83f0cb + f969d97 on top of bf3646c (BLOCK, comment 15358). Flips were executed in a fresh clone.
**Finding 1 (ReadScoped, `internal/notes/edit.go:83`): resolved.** I ran all 6 bypass variants (`os.OpenRoot().ReadFile`, `fs.ReadFile(root.FS())`, Lstat-only + root.ReadFile, Lstat + handle check with no SameFile, `os.ReadFile` after Lstat, handle check only). Each ran 3 times, and `TestReadScopedDoesNotReturnAnInBrainSymlinkTargetStaticallyOrDuringASampledSwapRace` went **red 18/18**, at `read_root_test.go:67` (the `AssertNoLeakFrom` race). For the two narrowest variants I preserved the size limit, so the only failure is the new test, without the over-limit size test also failing just because the bypass dropped the limit: **red 3/3** each. No SameFile leaked about 5k times in about 515k iterations; no Lstat leaked about 265k times in about 420k. Before this commit both variants were green 3/3.
- Note: the static assertions (:52-53) never fired under any variant. That is because `Resolve`'s Lstat refuses a static link before the read, so they pin `ReadScoped`'s behaviour, not `edit.go:83`. The test name describes the behaviour, so this is accurate.
**Finding 2 (rule 9): resolved.** `TestQueryDoesNotReturnAnInBrainSymlinkTargetDuringASampledSwapRace`, `TestHydrateRefusesAnInBrainSymlinkAndDoesNotReturnItsTargetDuringASampledSwapRace` and the ReadScoped name each claim no more than their assertions prove. The brainfstest doc now says "sampled window only".
**Findings 3 and 4 (non-blocking):** the coordinator is filing these as a follow-up issue.
**Flakiness:** unflipped `-count=5` on notes and search: ok.
**Gate:** `CI=1 GOFLAGS=-p=2 task check` at f969d97 gave **EXIT=0** and 0 FAIL lines, and the notes package ran.
**CI:** run **9330** on f969d97 succeeded (mirror job skipped). Mergeable. That green adds nothing beyond the flips above.
**Not checked:** hard-link targets, read paths outside brainfs, and pre-existing names on main (for example `TestQueryLeaksNothingUnderASymlinkSwapRace`), which are outside this PR's diff.
After main's #177 floor (AssertNoLeakFrom needs at least one read that
returns the inside note), this test went red on merge: the reader
queried only the sentinel, so the regular note could never be a hit
and the race had been passing vacuously. Query both terms.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Update after the round-3 PASS (comment 15390). Main gained #177 (0bd0fbf). With it, AssertNoLeakFrom fails unless at least one read returns the inside note. When this PR was merged onto main, TestQueryDoesNotReturnAnInBrainSymlinkTargetDuringASampledSwapRace went red with "no read in 45518 iterations returned the inside note".
The test reader queried only the sentinel, so the regular note could never be a hit. That means the race passed without exercising anything, which is the gap #177 exists to catch.
**Update after the round-3 PASS (comment 15390).** Main gained #177 (0bd0fbf). With it, `AssertNoLeakFrom` fails unless at least one read returns the inside note. When this PR was merged onto main, `TestQueryDoesNotReturnAnInBrainSymlinkTargetDuringASampledSwapRace` went red with "no read in 45518 iterations returned the inside note".
The test reader queried **only the sentinel**, so the regular note could never be a hit. That means the race passed without exercising anything, which is the gap #177 exists to catch.
**What changed:**
- **51183d6:** merges origin/main into this branch.
- **1cf05fd:** the reader now queries both terms.
**Checks:**
- Search package passes 3/3.
- `CI=1 task check` exits 0.
- The branch is now a fast-forward from f969d97.
- No production code changed.
**Still to do before merge:** confirm CI is green on 1cf05fd.
---
_Created via git-mcp on behalf of @mathias_
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.
Closes #171. Test-only (plus a brainfstest failure message). risk/high per the issue label, so it goes through a PR.
Gap: os.Root refuses links that point outside the brain, so the outside-target tests for slices 1–3 stayed green when a caller skipped brainfs's Lstat/SameFile checks.
Added, per caller (search.Query, hydrate, vectorstore.Sync, readExcerpt, api.ReadNote):
wiki/w/facts/inlink.md -> ../../../raw/s.mdbrainfstest.AssertNoLeakFromwith an in-brain targetFlip evidence (executed): I replaced each caller's
bfs.ReadRegular(x)withos.OpenRoot(brainDir).ReadFile(x). Every caller went red, each on the new assertions:TestQueryLeaksNothingFromAnInBrainTargetUnderASymlinkSwapRaceTestHydrateRefusesAnInBrainSymlink...Under Sync and Query, WalkRegular already skips a static link, so only the swap race catches those two bypasses.
Also:
TestReadRegularDoesNotReturnAnInBrainFileSwappedInBehindASymlinkAssertNoLeakFrom's message now names the targetGate:
CI=1 task checkEXIT=0, 0 FAILs.🤖 Generated with Claude Code
Created via git-mcp on behalf of @mathias
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:TestQueryLeaksNothingFromAnInBrainTargetUnderASymlinkSwapRaceandTestHydrateRefusesAnInBrainSymlinkAndLeaksNothingUnderASwapRace.TestReadRegularDoesNotReturnAnInBrainFileSwappedInBehindASymlink, e.g....DoesNotReturnAnInBrainTarget....Flip tests (executed, 5 callers x 3 bypasses)
os.OpenRoot(brainDir).ReadFile(the PR's claim)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=10on search, vectorstore, mcp, api and brainfs: EXIT=0, 0 FAIL, 0 DATA RACE.CI=1 task checkatbf3646c: EXIT=0, 0 FAIL. The run included the four ingestion packages and gitleaks.bf3646c: success.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:
AssertNoLeakFromwith an in-brain target per callerNon-blocking
rs, _ := Query(...)discards errors. The flip proves it observes a leak, but a Query that always errors would still pass it.brainfstestpackage doc still says "the outside file" for what can now be an in-brain target.Not checked
pr-gatekeeper verdict: BLOCK (hyperguild#172 @
bf3646c)Mode: PR. Reviewed by executing flips in a fresh clone (
~/.cache/hg-work/r172). I did not merge or close anything.Findings (most severe first)
internal/notes/edit.go:83:notes.ReadScoped(brain_read, slice 2, #165) is a slice 1–3 caller that still has no in-brain target. This is the gap class #171 exists to close. I bypassed the SameFile check there (Lstat, then the handle check, then read with no SameFile) andTestReadScopedRefusesSymlinksAndSurvivesSwapRacesstayed green 3/3. With the Lstat dropped as well, it was also green 3/3.internal/notes/read_root_test.goonly callsAssertNoLeak, whose target is outside the brain, and os.Root refuses that on its own. #171's list of five missed this sixth caller, and the PR copies that list. So the PR claim "each slice 1–3 caller" is false for 1 of 6. Fix: addbrainfstest.AssertNoLeakFrom(t, p, "inside", brainfstest.WriteSentinelAt(t, filepath.Join(brain, "raw", "s.md")), …, read)to that test, then flip it. Slice 4 (graphsync.go:71,wiki/inventory.go:55,wiki/index.go:60) is already covered: all three went red under both bypasses.TestQueryLeaksNothingFromAnInBrainTargetUnderASymlinkSwapRaceandTestHydrateRefuses…AndLeaksNothingUnderASwapRace. "LeaksNothing" is a universal claim about a race that is sampled for 1.5 s. That is the same overclaim #171 asked to remove from "Never", and this PR renames that one while adding two new ones. Honest names would be…DoesNotLeakAnInBrainTarget…/…AndDoesNotLeakItUnderASwapRace.search/brainroot_test.go:186-194: the Query race reader discards Query's error, and the test has no positive control. I pointedbrainfs.Openat a nonexistent dir so that every Query errors, and the new test stayed green (11 other tests in the package went red). Hydrate's test has the positive control and Query's does not.AssertNoLeakFromhas no minimum for iterations or for successful reads. If the swapper'sos.Symlinkfails, which it does silently because of_ =, the test passes without proving anything.Flip tests (executed: 5 listed sites × 6 bypass variants)
Variants:
os.OpenRoot(dir).ReadFile·fs.ReadFile(root.FS())· Lstat-only + root.ReadFile · Lstat + handle check with no SameFile ·os.ReadFileafter Lstat · handle check only with no Lstat.-count=5per site: 5/5 red at each site, with thousands of leaks per run. For example Sync leaked 4.6k–7.5k times in about 45k iterations, and ReadNote about 77k times in about 260k.-count=5on search, vectorstore, mcp, api and brainfs: all ok, no flakes.Gate / CI
CI=1 GOFLAGS=-p=2 task checkatbf3646c: EXIT=0, 0 FAIL lines, and the log shows the ingestion packages ran.bf3646c): success. Mirror job skipped. Mergeable.Not checked
Resolve's own tests (path_test.go) use an in-brain static target.-p=2.pr-gatekeeper re-review: PASS (hyperguild#172 @
f969d97). Not merged: risk/high, so Mathias merges.Mode: PR. Re-review of
a83f0cb+f969d97on top ofbf3646c(BLOCK, comment 15358). Flips were executed in a fresh clone.Finding 1 (ReadScoped,
internal/notes/edit.go:83): resolved. I ran all 6 bypass variants (os.OpenRoot().ReadFile,fs.ReadFile(root.FS()), Lstat-only + root.ReadFile, Lstat + handle check with no SameFile,os.ReadFileafter Lstat, handle check only). Each ran 3 times, andTestReadScopedDoesNotReturnAnInBrainSymlinkTargetStaticallyOrDuringASampledSwapRacewent red 18/18, atread_root_test.go:67(theAssertNoLeakFromrace). For the two narrowest variants I preserved the size limit, so the only failure is the new test, without the over-limit size test also failing just because the bypass dropped the limit: red 3/3 each. No SameFile leaked about 5k times in about 515k iterations; no Lstat leaked about 265k times in about 420k. Before this commit both variants were green 3/3.Resolve's Lstat refuses a static link before the read, so they pinReadScoped's behaviour, notedit.go:83. The test name describes the behaviour, so this is accurate.Finding 2 (rule 9): resolved.
TestQueryDoesNotReturnAnInBrainSymlinkTargetDuringASampledSwapRace,TestHydrateRefusesAnInBrainSymlinkAndDoesNotReturnItsTargetDuringASampledSwapRaceand the ReadScoped name each claim no more than their assertions prove. The brainfstest doc now says "sampled window only".Findings 3 and 4 (non-blocking): the coordinator is filing these as a follow-up issue.
Flakiness: unflipped
-count=5on notes and search: ok.Gate:
CI=1 GOFLAGS=-p=2 task checkatf969d97gave EXIT=0 and 0 FAIL lines, and the notes package ran.CI: run 9330 on
f969d97succeeded (mirror job skipped). Mergeable. That green adds nothing beyond the flips above.Not checked: hard-link targets, read paths outside brainfs, and pre-existing names on main (for example
TestQueryLeaksNothingUnderASymlinkSwapRace), which are outside this PR's diff.Update after the round-3 PASS (comment 15390). Main gained #177 (
0bd0fbf). With it,AssertNoLeakFromfails unless at least one read returns the inside note. When this PR was merged onto main,TestQueryDoesNotReturnAnInBrainSymlinkTargetDuringASampledSwapRacewent red with "no read in 45518 iterations returned the inside note".The test reader queried only the sentinel, so the regular note could never be a hit. That means the race passed without exercising anything, which is the gap #177 exists to catch.
What changed:
51183d6: merges origin/main into this branch.1cf05fd: the reader now queries both terms.Checks:
CI=1 task checkexits 0.f969d97.Still to do before merge: confirm CI is green on
1cf05fd.Created via git-mcp on behalf of @mathias