Fixes#158 (part B; part A was #189). risk/high: control code, adversarial review required.
Problem
These writers still created or rewrote brain files by path:
pipeline buildAndWrite:os.MkdirAll + os.WriteFile of every page.
BackfillRefs:os.WriteFile of each concept page. A page swapped for a symlink after the read was written through. The new race test overwrote an in-brain target before this change.
watcher:
the raw/processed and raw/failed archive copies (os.Create)
the .processed/.failed markers
the log.md entry (os.OpenFile)
cmd/server: the claude-session archive.
Change
All of these now go through brainfs.PutRegular/AppendRegular.
New package-level brainfs.PutFile/AppendFile cover one-off writers. The watcher file's call-site lint bans .Open calls there so raw/ reads stay on OpenRegular, and these helpers keep that intact.
Tests (red before the fix)
TestProcessDirDoesNotWriteThroughASymlinkedArchiveDirOrLog, for both processed and failed. A pre-planted marker symlink makes the watcher skip the note, so the marker write itself is reachable only by a race.
TestBackfillRefsNeverWritesThroughAPageSwappedForASymlink: the swap race, with a positive control.
Not covered
Static redirects of pipeline writes: most are already refused by the reads hardened in #176 and #187 before any write happens. This PR is defence in depth against races.
Residual: a directory swapped in after the checks is #167.
Fixes #158 (part B; part A was #189). risk/high: control code, adversarial review required.
## Problem
These writers still created or rewrote brain files by path:
- **pipeline `buildAndWrite`:** `os.MkdirAll` + `os.WriteFile` of every page.
- **`BackfillRefs`:** `os.WriteFile` of each concept page. A page swapped for a symlink after the read was written through. The new race test overwrote an in-brain target before this change.
- **watcher:**
- the `raw/processed` and `raw/failed` archive copies (`os.Create`)
- the `.processed`/`.failed` markers
- the `log.md` entry (`os.OpenFile`)
- **`cmd/server`:** the claude-session archive.
## Change
- All of these now go through `brainfs.PutRegular`/`AppendRegular`.
- New package-level `brainfs.PutFile`/`AppendFile` cover one-off writers. The watcher file's call-site lint bans `.Open` calls there so `raw/` reads stay on `OpenRegular`, and these helpers keep that intact.
## Tests (red before the fix)
- `TestProcessDirDoesNotWriteThroughASymlinkedArchiveDirOrLog`, for both processed and failed. A pre-planted marker symlink makes the watcher skip the note, so the marker write itself is reachable only by a race.
- `TestProcessDirWritesMarkersAndArchiveInRealDirectories`: positive control.
- `TestBackfillRefsNeverWritesThroughAPageSwappedForASymlink`: the swap race, with a positive control.
## Not covered
- **Static redirects of pipeline writes:** most are already refused by the reads hardened in #176 and #187 before any write happens. This PR is defence in depth against races.
- **Residual:** a directory swapped in after the checks is #167.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---
_Created via git-mcp on behalf of @mathias_
The remaining writers created or rewrote brain files by path:
- pipeline buildAndWrite: os.MkdirAll + os.WriteFile of every page.
- BackfillRefs: os.WriteFile of each concept page; a page swapped for a
symlink after the read was written through (the race test overwrote an
in-brain target before this change).
- watcher: the raw/processed and raw/failed archive copies (os.Create),
the .processed/.failed markers and the log.md entry (os.OpenFile).
- cmd/server: the claude-session archive.
All now go through brainfs PutRegular/AppendRegular (new package-level
PutFile/AppendFile for one-off writers: the watcher's call-site lint bans
.Open calls in that file so raw/ reads stay on OpenRegular).
Stacked on #189 (needs PutRegular/AppendRegular).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Verified by execution against a scratch copy of the branch. CI run 9518 is green and CI=1 task check exits 0. Neither covers the findings below.
Blocking
The log.md write has no test that reaches it. The test is named TestProcessDirDoesNotWriteThroughASymlinkedArchiveDirOrLog. In its failed subtest, raw/failed is symlinked, so copyFromRoot fails and processFile returns before it gets to appendWatcherLog. As a flip, I put the by-path os.OpenFile append back inside appendWatcherLog, and every test stayed green (watcher + extract). The call-site lint doesn't catch it either, because internal/extract/callsites_test.go still allowlists watcher.go:appendWatcherLog for os.Open*, even though that function no longer opens anything. So the name claims the log is covered, and the test cannot reach the log (rule 9). Fix: test log.md with a real raw/failed, and remove the stale allowlist entry.
The marker writes can be reached without a race, contrary to the test comment, and no test checks them. The comment says a pre-planted marker symlink makes the watcher skip the note. That is false for a dangling symlink: os.Stat(path+".processed") follows it, gets ENOENT, and the note is processed. As a flip, I put both .processed and .failed back to os.WriteFile(path+...). All tests stayed green, and in that state a dangling marker link creates its target outside the brain. The PR's own code refuses this correctly, but nothing asserts that it does.
When a marker write is refused, the watcher loops forever. With the PR code and a dangling .processed link, each interval writes a new timestamped archive copy into raw/processed/<date>/: 4 rounds gave 4 copies. At the default 30s interval that is about 2,880 files a day per note, in a folder Syncthing syncs. With a dangling .failed link, the LLM is called again every interval, and raw/failed/<f> plus log.md are rewritten or appended each time. That is unbounded model spend plus log growth. Before this PR the marker followed the link, which was the bug, but it did stop the loop. A refusal has to end in a terminal state, not a retry.
When an archive copy is refused, the note is stuck and retried every interval. This happens with a symlinked raw/failed or a FIFO at raw/failed/<f>: no .failed marker gets written, so the LLM is called again every interval (3 calls in 3 rounds). It also happens with a symlinked raw/processed or raw/processed/<date>: an error every interval, cheap only because of the body-hash early return. And it happens with no attacker for a note whose filename contains a backslash, which is legal on Linux and macOS. cleanRel refuses it, so the note is never archived or marked and errors every interval. Before this PR that note was handled normally.
Two converted writers have no assertion at all. For buildAndWrite (pipeline.gowriteBrainFile), I reverted it to MkdirAll + os.WriteFile and go test ./... stayed green. Pre-planted shapes are already refused by the inventory/reviseOrCreate reads, so this write only matters under a read-to-write swap, and no race test covers it. For the cmd/server claude-session archive, I reverted it to MkdirAll + os.WriteFile and go test ./... stayed green. In both cases the code is correct but nothing would notice if it were removed.
Non-blocking
copyFromRoot now does io.ReadAll on the raw file. For .md/.txt that doubles peak memory, since TextIn already reads the whole file. For .pdf it is new: previously both the extraction and the archive streamed it. There is no upstream size cap. Consider a streaming or size-capped PutRegular.
The claude-session sink refuses an unclean host or session id (verified: .., empty host, backslash; a/b nests and stays inside the archive). The refusal returns an error, so the claudewatcher cursor never advances and that JSONL is retried every tick. The ids are a Claude Code UUID and the hostname, so this is low risk.
The BackfillRefs swap test is good: reverting to os.WriteFile turned it red 3 of 3 times. Its ok control counts nil returns, not writes.
What passed
The probes (watcher end to end via processDir, pipeline RunRaw, claudeSink.Ingest) covered a symlinked raw/processed, raw/failed and raw/processed/, both in-brain and outside; a symlinked log.md (with a real raw/failed); FIFOs at the archive, marker and log paths; and a symlinked or FIFO page. In every case nothing was created at a link target and nothing hung. -race -count=3 passed on watcher, pipeline and brainfs.
Label is risk/high: merge is Mathias's call, so this is not a merge request.
## pr-gatekeeper review — PR #195 (head 77c44f2): BLOCKED
Verified by execution against a scratch copy of the branch. CI run 9518 is green and `CI=1 task check` exits 0. Neither covers the findings below.
### Blocking
1. **The log.md write has no test that reaches it.** The test is named `TestProcessDirDoesNotWriteThroughASymlinkedArchiveDirOrLog`. In its `failed` subtest, `raw/failed` is symlinked, so `copyFromRoot` fails and `processFile` returns before it gets to `appendWatcherLog`. As a flip, I put the by-path `os.OpenFile` append back inside `appendWatcherLog`, and every test stayed green (watcher + extract). The call-site lint doesn't catch it either, because `internal/extract/callsites_test.go` still allowlists `watcher.go:appendWatcherLog` for `os.Open*`, even though that function no longer opens anything. So the name claims the log is covered, and the test cannot reach the log (rule 9). Fix: test log.md with a real `raw/failed`, and remove the stale allowlist entry.
2. **The marker writes can be reached without a race, contrary to the test comment, and no test checks them.** The comment says a pre-planted marker symlink makes the watcher skip the note. That is false for a *dangling* symlink: `os.Stat(path+".processed")` follows it, gets ENOENT, and the note is processed. As a flip, I put both `.processed` and `.failed` back to `os.WriteFile(path+...)`. All tests stayed green, and in that state a dangling marker link creates its target outside the brain. The PR's own code refuses this correctly, but nothing asserts that it does.
3. **When a marker write is refused, the watcher loops forever.** With the PR code and a dangling `.processed` link, each interval writes a *new* timestamped archive copy into `raw/processed/<date>/`: 4 rounds gave 4 copies. At the default 30s interval that is about 2,880 files a day per note, in a folder Syncthing syncs. With a dangling `.failed` link, the LLM is called again every interval, and `raw/failed/<f>` plus `log.md` are rewritten or appended each time. That is unbounded model spend plus log growth. Before this PR the marker followed the link, which was the bug, but it did stop the loop. A refusal has to end in a terminal state, not a retry.
4. **When an archive copy is refused, the note is stuck and retried every interval.** This happens with a symlinked `raw/failed` or a FIFO at `raw/failed/<f>`: no `.failed` marker gets written, so the LLM is called again every interval (3 calls in 3 rounds). It also happens with a symlinked `raw/processed` or `raw/processed/<date>`: an error every interval, cheap only because of the body-hash early return. And it happens with **no attacker** for a note whose filename contains a backslash, which is legal on Linux and macOS. `cleanRel` refuses it, so the note is never archived or marked and errors every interval. Before this PR that note was handled normally.
5. **Two converted writers have no assertion at all.** For `buildAndWrite` (`pipeline.go` `writeBrainFile`), I reverted it to `MkdirAll` + `os.WriteFile` and `go test ./...` stayed green. Pre-planted shapes are already refused by the inventory/`reviseOrCreate` reads, so this write only matters under a read-to-write swap, and no race test covers it. For the `cmd/server` claude-session archive, I reverted it to `MkdirAll` + `os.WriteFile` and `go test ./...` stayed green. In both cases the code is correct but nothing would notice if it were removed.
### Non-blocking
- `copyFromRoot` now does `io.ReadAll` on the raw file. For `.md`/`.txt` that doubles peak memory, since `TextIn` already reads the whole file. For `.pdf` it is new: previously both the extraction and the archive streamed it. There is no upstream size cap. Consider a streaming or size-capped `PutRegular`.
- The claude-session sink refuses an unclean host or session id (verified: `..`, empty host, backslash; `a/b` nests and stays inside the archive). The refusal returns an error, so the claudewatcher cursor never advances and that JSONL is retried every tick. The ids are a Claude Code UUID and the hostname, so this is low risk.
- The BackfillRefs swap test is good: reverting to `os.WriteFile` turned it red 3 of 3 times. Its `ok` control counts nil returns, not writes.
### What passed
The probes (watcher end to end via `processDir`, pipeline `RunRaw`, `claudeSink.Ingest`) covered a symlinked raw/processed, raw/failed and raw/processed/<date>, both in-brain and outside; a symlinked log.md (with a real raw/failed); FIFOs at the archive, marker and log paths; and a symlinked or FIFO page. In every case nothing was created at a link target and nothing hung. `-race -count=3` passed on watcher, pipeline and brainfs.
Label is `risk/high`: merge is Mathias's call, so this is not a merge request.
Review round 1 on PR #195:
- A refused marker or archive write left no marker, so the note was
reprocessed every interval (archive copies piling up; an LLM call per
round on the failure path). The marker check now uses Lstat (any entry
at the marker path counts, so a dangling marker link skips the note
instead of looping), and a note whose archive copy or marker write is
refused is remembered (path, size, mtime) and not retried until it
changes.
- cleanRel no longer refuses a backslash: it is a legal filename byte on
Linux, not a traversal, and a raw note named with one was never
archived or marked.
- Tests now reach every converted writer: the log.md append (real
raw/failed so the log is reached, symlinked log.md), a dangling marker
link, the refused-retry loop with a positive control, a backslash name,
a buildAndWrite swap race, and the claude-session archive with in-brain
and outside symlinked archive dirs. The stale lint allowlist entry for
appendWatcherLog is removed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
All findings below were verified by execution on a scratch copy of the branch. CI run 9538 is green. CI=1 task check exits 0. -race -count=3 passes on watcher, pipeline, brainfs and cmd/server. Green covers none of the blocking finding.
Round-1 findings now closed (each flip goes red)
Log append. An inline by-path os.OpenFile turns both TestProcessDirDoesNotAppendTheLogThroughASymlink and the call-site lint red.
.processed marker check. Changing Lstat back to Stat turns TestProcessDirTreatsAnyEntryAtTheMarkerPathAsAMarker red.
Refused-note memory. Removing the skip, removing the remember, or ignoring changes each turns TestProcessDirDoesNotRetryANoteWhoseArchiveCopyIsRefused red.
Backslash. Restoring the refusal turns TestProcessDirProcessesANoteWithABackslashInItsName red.
buildAndWrite by path.TestBuildAndWriteNeverWritesThroughAPageSwappedForASymlink goes red, 3 of 3.
claudeSink by path.TestClaudeSinkDoesNotWriteThroughASymlinkedArchiveDir goes red.
Archive copy by path. Two tests go red.
Blocking
No test covers the marker writes, or the new rule that a refused marker write is final. It now takes a race to reach the marker write. But the race window is the whole pipeline.Run, so in production it lasts as long as an LLM call. A test can hit it deterministically: plant the entry at the marker path from inside the fake Complete. I verified that against the PR code. Both .processed and .failed are refused, nothing is created at the link target, and the note is remembered (1 call in 3 rounds).
No test in the PR does this, so these flips all stay green:
.processed or .failed marker written back by path (os.WriteFile(path+...)). This is the #158 write-through itself. The probe above shows a file created at the link target.
The .failed refusal return dropped back to log-only. Suppose the planted marker entry is then removed (symlinkguard removes such entries). The LLM is called again every round: 3 calls in 3 rounds, which is the round-1 loop. The PR fixes this in code but has no test for it.
The .processed refusal return dropped back to log-only. Also green.
Fix: add a watcher test that plants the marker entry during Complete, for both .processed and .failed. Assert that nothing is created at the target, and that the note is not retried after the entry goes away.
Non-blocking
.failed check reverted to Stat. This stays green. The refused-note memory contains the result: 1 call, nothing written outside. A one-line test case would pin it.
cleanRel no longer refuses \. That is right on Linux. But it loosens a shared primitive behind every brainfs reader and writer, not just the watcher. No brainfs-level test pins either behaviour. A test should state the intent, so nobody "restores" the refusal later.
Raw archive copy reads the whole file into memory (io.ReadAll). It is still not streamed. For .md/.txt this doubles peak memory, since TextIn already reads the whole file. For .pdf it is a new full in-memory copy, with no upstream size cap. Not blocking; file it as a follow-up (stream or cap).
Positive-control message in the buildAndWrite test. "some runs wrote the page" counts nil returns, not writes (rule 9, minor).
Not this PR (main has it too): a marker write that fails with anything other than ErrNotRegular is only logged, so the note is not remembered. ENAMETOOLONG from a near-255-byte note name is one example. Deduced from the code, not executed.
risk/high: the merge is Mathias's call.
## pr-gatekeeper review, round 2: PR #195 (head 1a2a93d): BLOCKED
All findings below were verified by execution on a scratch copy of the branch. CI run 9538 is green. `CI=1 task check` exits 0. `-race -count=3` passes on watcher, pipeline, brainfs and cmd/server. Green covers none of the blocking finding.
### Round-1 findings now closed (each flip goes red)
- **Log append.** An inline by-path `os.OpenFile` turns both `TestProcessDirDoesNotAppendTheLogThroughASymlink` and the call-site lint red.
- **`.processed` marker check.** Changing Lstat back to Stat turns `TestProcessDirTreatsAnyEntryAtTheMarkerPathAsAMarker` red.
- **Refused-note memory.** Removing the skip, removing the remember, or ignoring changes each turns `TestProcessDirDoesNotRetryANoteWhoseArchiveCopyIsRefused` red.
- **Backslash.** Restoring the refusal turns `TestProcessDirProcessesANoteWithABackslashInItsName` red.
- **buildAndWrite by path.** `TestBuildAndWriteNeverWritesThroughAPageSwappedForASymlink` goes red, 3 of 3.
- **claudeSink by path.** `TestClaudeSinkDoesNotWriteThroughASymlinkedArchiveDir` goes red.
- **Archive copy by path.** Two tests go red.
### Blocking
1. **No test covers the marker writes, or the new rule that a refused marker write is final.** It now takes a race to reach the marker write. But the race window is the whole `pipeline.Run`, so in production it lasts as long as an LLM call. A test can hit it deterministically: plant the entry at the marker path from inside the fake `Complete`. I verified that against the PR code. Both `.processed` and `.failed` are refused, nothing is created at the link target, and the note is remembered (1 call in 3 rounds).
No test in the PR does this, so these flips all stay green:
- **`.processed` or `.failed` marker written back by path (`os.WriteFile(path+...)`).** This is the #158 write-through itself. The probe above shows a file created at the link target.
- **The `.failed` refusal return dropped back to log-only.** Suppose the planted marker entry is then removed (symlinkguard removes such entries). The LLM is called again every round: 3 calls in 3 rounds, which is the round-1 loop. The PR fixes this in code but has no test for it.
- **The `.processed` refusal return dropped back to log-only.** Also green.
Fix: add a watcher test that plants the marker entry during `Complete`, for both `.processed` and `.failed`. Assert that nothing is created at the target, and that the note is not retried after the entry goes away.
### Non-blocking
- **`.failed` check reverted to Stat.** This stays green. The refused-note memory contains the result: 1 call, nothing written outside. A one-line test case would pin it.
- **`cleanRel` no longer refuses `\`.** That is right on Linux. But it loosens a shared primitive behind every brainfs reader and writer, not just the watcher. No brainfs-level test pins either behaviour. A test should state the intent, so nobody "restores" the refusal later.
- **Raw archive copy reads the whole file into memory (`io.ReadAll`).** It is still not streamed. For `.md`/`.txt` this doubles peak memory, since TextIn already reads the whole file. For `.pdf` it is a new full in-memory copy, with no upstream size cap. Not blocking; file it as a follow-up (stream or cap).
- **Positive-control message in the buildAndWrite test.** "some runs wrote the page" counts nil returns, not writes (rule 9, minor).
- **Not this PR (main has it too):** a marker write that fails with anything other than ErrNotRegular is only logged, so the note is not remembered. ENAMETOOLONG from a near-255-byte note name is one example. Deduced from the code, not executed.
`risk/high`: the merge is Mathias's call.
Review round 2 on PR #195: the marker writes had no test, so reverting
them to by-path writes, or the refusal to log-only, stayed green. The
link is planted from inside the fake model call, between the walk's
Lstat and the marker write, for both markers and both an outside and
an in-brain relative target; red on all four flips. The marker-entry
skip is now checked for .failed too.
Also pins cleanRel's acceptance of a backslash as a filename byte in
brainfs itself, and stops the buildAndWrite control claiming writes it
does not count.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
All results below come from running the code on a scratch copy of the branch. The round-3 diff changes tests only (3 test files); production code is the same as round 2.
Round-2 blocker: closed
TestProcessDirRefusesAMarkerLinkPlantedDuringTheRunAndDoesNotRetry plants the marker link during the run, as the round-2 review asked. Each of these flips turns it red:
.processed written by path: red on the processed/outside and processed/in-brain subtests
.failed written by path: red on both failed/* subtests
.processed refusal changed to log-only
.failed refusal changed to log-only
refused-note remember removed
refused-note skip removed
Every flip in the suite, re-run at this head
Each turns at least one test red:
.processed or .failed check changed from Lstat to Stat
backslash refusal put back into cleanRel (red: TestReadAndPutRegularTreatABackslashAsAFilenameByte)
log append inline by path (red: the log test and the call-site lint)
archive copy written by path
buildAndWrite written by path
BackfillRefs written by path
claudeSink written by path
End-to-end probes, re-run at this head
I ran processDir with a fake pipeline against nine planted shapes:
a symlinked raw/processed (outside target and in-brain target)
a symlinked raw/processed/
a symlinked raw/failed
a FIFO at each archive path
a FIFO marker
a symlinked log.md
a FIFO log.md
Results over 3 rounds per shape:
nothing was written at any link target
nothing hung
a refused failed-path note made 1 LLM call (round 1: 3)
Gates
CI run 9547 (Lint / Test / Vet) on 22d0653: success
CI=1 task check: exit 0, with 0 --- FAIL lines
-race -count=3 on watcher, pipeline, brainfs, cmd/server and extract: ok
PR is mergeable
Follow-ups the coordinator is filing as issues (non-blocking)
io.ReadAll memory in the raw archive copy (.pdf has no size cap)
ENAMETOOLONG marker loop, which is already on main
Not checked
Hard links and a directory swapped after the checks (the #167 residuals)
The pod's memory limit
Whether symlinkguard really removes a marker link in production
Label is risk/high, so the merge is Mathias's call, because merging deploys ingestion. This review does not merge.
## pr-gatekeeper review, round 3: PR #195 (head 22d0653): PASS
All results below come from running the code on a scratch copy of the branch. The round-3 diff changes tests only (3 test files); production code is the same as round 2.
### Round-2 blocker: closed
`TestProcessDirRefusesAMarkerLinkPlantedDuringTheRunAndDoesNotRetry` plants the marker link during the run, as the round-2 review asked. Each of these flips turns it red:
- `.processed` written by path: red on the `processed/outside` and `processed/in-brain` subtests
- `.failed` written by path: red on both `failed/*` subtests
- `.processed` refusal changed to log-only
- `.failed` refusal changed to log-only
- refused-note remember removed
- refused-note skip removed
### Every flip in the suite, re-run at this head
Each turns at least one test red:
- `.processed` or `.failed` check changed from Lstat to Stat
- backslash refusal put back into `cleanRel` (red: `TestReadAndPutRegularTreatABackslashAsAFilenameByte`)
- log append inline by path (red: the log test and the call-site lint)
- archive copy written by path
- buildAndWrite written by path
- BackfillRefs written by path
- claudeSink written by path
### End-to-end probes, re-run at this head
I ran `processDir` with a fake pipeline against nine planted shapes:
- a symlinked raw/processed (outside target and in-brain target)
- a symlinked raw/processed/<date>
- a symlinked raw/failed
- a FIFO at each archive path
- a FIFO marker
- a symlinked log.md
- a FIFO log.md
Results over 3 rounds per shape:
- nothing was written at any link target
- nothing hung
- a refused failed-path note made 1 LLM call (round 1: 3)
### Gates
- CI run 9547 (Lint / Test / Vet) on 22d0653: success
- `CI=1 task check`: exit 0, with 0 `--- FAIL` lines
- `-race -count=3` on watcher, pipeline, brainfs, cmd/server and extract: ok
- PR is mergeable
### Follow-ups the coordinator is filing as issues (non-blocking)
- `io.ReadAll` memory in the raw archive copy (`.pdf` has no size cap)
- ENAMETOOLONG marker loop, which is already on main
### Not checked
- Hard links and a directory swapped after the checks (the #167 residuals)
- The pod's memory limit
- Whether symlinkguard really removes a marker link in production
Label is `risk/high`, so the merge is Mathias's call, because merging deploys ingestion. This review does not merge.
All checks were successful
CI / Lint / Test / Vet (pull_request) Successful in 1m3s
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
Fixes #158 (part B; part A was #189). risk/high: control code, adversarial review required.
Problem
These writers still created or rewrote brain files by path:
buildAndWrite:os.MkdirAll+os.WriteFileof every page.BackfillRefs:os.WriteFileof each concept page. A page swapped for a symlink after the read was written through. The new race test overwrote an in-brain target before this change.raw/processedandraw/failedarchive copies (os.Create).processed/.failedmarkerslog.mdentry (os.OpenFile)cmd/server: the claude-session archive.Change
brainfs.PutRegular/AppendRegular.brainfs.PutFile/AppendFilecover one-off writers. The watcher file's call-site lint bans.Opencalls there soraw/reads stay onOpenRegular, and these helpers keep that intact.Tests (red before the fix)
TestProcessDirDoesNotWriteThroughASymlinkedArchiveDirOrLog, for both processed and failed. A pre-planted marker symlink makes the watcher skip the note, so the marker write itself is reachable only by a race.TestProcessDirWritesMarkersAndArchiveInRealDirectories: positive control.TestBackfillRefsNeverWritesThroughAPageSwappedForASymlink: the swap race, with a positive control.Not covered
🤖 Generated with Claude Code
Created via git-mcp on behalf of @mathias
pr-gatekeeper review — PR #195 (head
77c44f2): BLOCKEDVerified by execution against a scratch copy of the branch. CI run 9518 is green and
CI=1 task checkexits 0. Neither covers the findings below.Blocking
The log.md write has no test that reaches it. The test is named
TestProcessDirDoesNotWriteThroughASymlinkedArchiveDirOrLog. In itsfailedsubtest,raw/failedis symlinked, socopyFromRootfails andprocessFilereturns before it gets toappendWatcherLog. As a flip, I put the by-pathos.OpenFileappend back insideappendWatcherLog, and every test stayed green (watcher + extract). The call-site lint doesn't catch it either, becauseinternal/extract/callsites_test.gostill allowlistswatcher.go:appendWatcherLogforos.Open*, even though that function no longer opens anything. So the name claims the log is covered, and the test cannot reach the log (rule 9). Fix: test log.md with a realraw/failed, and remove the stale allowlist entry.The marker writes can be reached without a race, contrary to the test comment, and no test checks them. The comment says a pre-planted marker symlink makes the watcher skip the note. That is false for a dangling symlink:
os.Stat(path+".processed")follows it, gets ENOENT, and the note is processed. As a flip, I put both.processedand.failedback toos.WriteFile(path+...). All tests stayed green, and in that state a dangling marker link creates its target outside the brain. The PR's own code refuses this correctly, but nothing asserts that it does.When a marker write is refused, the watcher loops forever. With the PR code and a dangling
.processedlink, each interval writes a new timestamped archive copy intoraw/processed/<date>/: 4 rounds gave 4 copies. At the default 30s interval that is about 2,880 files a day per note, in a folder Syncthing syncs. With a dangling.failedlink, the LLM is called again every interval, andraw/failed/<f>pluslog.mdare rewritten or appended each time. That is unbounded model spend plus log growth. Before this PR the marker followed the link, which was the bug, but it did stop the loop. A refusal has to end in a terminal state, not a retry.When an archive copy is refused, the note is stuck and retried every interval. This happens with a symlinked
raw/failedor a FIFO atraw/failed/<f>: no.failedmarker gets written, so the LLM is called again every interval (3 calls in 3 rounds). It also happens with a symlinkedraw/processedorraw/processed/<date>: an error every interval, cheap only because of the body-hash early return. And it happens with no attacker for a note whose filename contains a backslash, which is legal on Linux and macOS.cleanRelrefuses it, so the note is never archived or marked and errors every interval. Before this PR that note was handled normally.Two converted writers have no assertion at all. For
buildAndWrite(pipeline.gowriteBrainFile), I reverted it toMkdirAll+os.WriteFileandgo test ./...stayed green. Pre-planted shapes are already refused by the inventory/reviseOrCreatereads, so this write only matters under a read-to-write swap, and no race test covers it. For thecmd/serverclaude-session archive, I reverted it toMkdirAll+os.WriteFileandgo test ./...stayed green. In both cases the code is correct but nothing would notice if it were removed.Non-blocking
copyFromRootnow doesio.ReadAllon the raw file. For.md/.txtthat doubles peak memory, sinceTextInalready reads the whole file. For.pdfit is new: previously both the extraction and the archive streamed it. There is no upstream size cap. Consider a streaming or size-cappedPutRegular..., empty host, backslash;a/bnests and stays inside the archive). The refusal returns an error, so the claudewatcher cursor never advances and that JSONL is retried every tick. The ids are a Claude Code UUID and the hostname, so this is low risk.os.WriteFileturned it red 3 of 3 times. Itsokcontrol counts nil returns, not writes.What passed
The probes (watcher end to end via
processDir, pipelineRunRaw,claudeSink.Ingest) covered a symlinked raw/processed, raw/failed and raw/processed/, both in-brain and outside; a symlinked log.md (with a real raw/failed); FIFOs at the archive, marker and log paths; and a symlinked or FIFO page. In every case nothing was created at a link target and nothing hung.-race -count=3passed on watcher, pipeline and brainfs.Label is
risk/high: merge is Mathias's call, so this is not a merge request.pr-gatekeeper review, round 2: PR #195 (head
1a2a93d): BLOCKEDAll findings below were verified by execution on a scratch copy of the branch. CI run 9538 is green.
CI=1 task checkexits 0.-race -count=3passes on watcher, pipeline, brainfs and cmd/server. Green covers none of the blocking finding.Round-1 findings now closed (each flip goes red)
os.OpenFileturns bothTestProcessDirDoesNotAppendTheLogThroughASymlinkand the call-site lint red..processedmarker check. Changing Lstat back to Stat turnsTestProcessDirTreatsAnyEntryAtTheMarkerPathAsAMarkerred.TestProcessDirDoesNotRetryANoteWhoseArchiveCopyIsRefusedred.TestProcessDirProcessesANoteWithABackslashInItsNamered.TestBuildAndWriteNeverWritesThroughAPageSwappedForASymlinkgoes red, 3 of 3.TestClaudeSinkDoesNotWriteThroughASymlinkedArchiveDirgoes red.Blocking
No test covers the marker writes, or the new rule that a refused marker write is final. It now takes a race to reach the marker write. But the race window is the whole
pipeline.Run, so in production it lasts as long as an LLM call. A test can hit it deterministically: plant the entry at the marker path from inside the fakeComplete. I verified that against the PR code. Both.processedand.failedare refused, nothing is created at the link target, and the note is remembered (1 call in 3 rounds).No test in the PR does this, so these flips all stay green:
.processedor.failedmarker written back by path (os.WriteFile(path+...)). This is the #158 write-through itself. The probe above shows a file created at the link target..failedrefusal return dropped back to log-only. Suppose the planted marker entry is then removed (symlinkguard removes such entries). The LLM is called again every round: 3 calls in 3 rounds, which is the round-1 loop. The PR fixes this in code but has no test for it..processedrefusal return dropped back to log-only. Also green.Fix: add a watcher test that plants the marker entry during
Complete, for both.processedand.failed. Assert that nothing is created at the target, and that the note is not retried after the entry goes away.Non-blocking
.failedcheck reverted to Stat. This stays green. The refused-note memory contains the result: 1 call, nothing written outside. A one-line test case would pin it.cleanRelno longer refuses\. That is right on Linux. But it loosens a shared primitive behind every brainfs reader and writer, not just the watcher. No brainfs-level test pins either behaviour. A test should state the intent, so nobody "restores" the refusal later.io.ReadAll). It is still not streamed. For.md/.txtthis doubles peak memory, since TextIn already reads the whole file. For.pdfit is a new full in-memory copy, with no upstream size cap. Not blocking; file it as a follow-up (stream or cap).risk/high: the merge is Mathias's call.pr-gatekeeper review, round 3: PR #195 (head
22d0653): PASSAll results below come from running the code on a scratch copy of the branch. The round-3 diff changes tests only (3 test files); production code is the same as round 2.
Round-2 blocker: closed
TestProcessDirRefusesAMarkerLinkPlantedDuringTheRunAndDoesNotRetryplants the marker link during the run, as the round-2 review asked. Each of these flips turns it red:.processedwritten by path: red on theprocessed/outsideandprocessed/in-brainsubtests.failedwritten by path: red on bothfailed/*subtests.processedrefusal changed to log-only.failedrefusal changed to log-onlyEvery flip in the suite, re-run at this head
Each turns at least one test red:
.processedor.failedcheck changed from Lstat to StatcleanRel(red:TestReadAndPutRegularTreatABackslashAsAFilenameByte)End-to-end probes, re-run at this head
I ran
processDirwith a fake pipeline against nine planted shapes:Results over 3 rounds per shape:
Gates
22d0653: successCI=1 task check: exit 0, with 0--- FAILlines-race -count=3on watcher, pipeline, brainfs, cmd/server and extract: okFollow-ups the coordinator is filing as issues (non-blocking)
io.ReadAllmemory in the raw archive copy (.pdfhas no size cap)Not checked
Label is
risk/high, so the merge is Mathias's call, because merging deploys ingestion. This review does not merge.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.