Fixes#192. risk/high: control code, adversarial review required.
Problem
brain_edit did its own I/O by path around notes.Edit:
Snapshot: taken with os.ReadFile, which hung on a FIFO note held open by a writer.
Rollback: written with os.WriteFile, by path.
Lock:<note>.lock was opened with O_CREATE by path. A planted dangling .lock symlink made a successful edit create a file at the link's target, outside the brain or elsewhere in it.
Change
The snapshot uses brainfs.ReadRegular, and the rollback uses brainfs.WriteRegular.
New brainfs.OpenLock / RemoveLock:
clean path, and real parent directories
an existing lock must be a regular file
the handle is opened with O_NOFOLLOW|O_NONBLOCK, then confirmed with SameFile. os.Root follows an in-root final symlink even with O_NOFOLLOW.
A dangling or swapped-in link is refused, and nothing is created at its target.
Tests (red before the fix)
TestBrainEditReturnsPromptlyOnAFIFONoteWithAWriter, with a positive control.
TestBrainEditDoesNotCreateAFileThroughADanglingLockSymlink: outside and in-brain targets.
Fixes #192. risk/high: control code, adversarial review required.
## Problem
`brain_edit` did its own I/O by path around `notes.Edit`:
- **Snapshot:** taken with `os.ReadFile`, which **hung on a FIFO note held open by a writer**.
- **Rollback:** written with `os.WriteFile`, by path.
- **Lock:** `<note>.lock` was opened with `O_CREATE` by path. A planted dangling `.lock` symlink made a **successful edit create a file at the link's target**, outside the brain or elsewhere in it.
## Change
- The snapshot uses `brainfs.ReadRegular`, and the rollback uses `brainfs.WriteRegular`.
- **New `brainfs.OpenLock` / `RemoveLock`:**
- clean path, and real parent directories
- an existing lock must be a regular file
- the handle is opened with `O_NOFOLLOW|O_NONBLOCK`, then confirmed with `SameFile`. os.Root follows an in-root final symlink even with `O_NOFOLLOW`.
- A dangling or swapped-in link is refused, and nothing is created at its target.
## Tests (red before the fix)
- `TestBrainEditReturnsPromptlyOnAFIFONoteWithAWriter`, with a positive control.
- `TestBrainEditDoesNotCreateAFileThroughADanglingLockSymlink`: outside and in-brain targets.
## Flips (red)
- Snapshot via `os.ReadFile`
- Lock opened by path
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---
_Created via git-mcp on behalf of @mathias_
Head 47354dc. Reviewed by execution against a scratch copy; tests mutated one guard at a time. Issue #192 is labelled risk/high, so this goes to Mathias for the merge regardless.
Blocking
The rollback hardening has no assertion. If bfs.WriteRegular(target.Rel, prior) in internal/mcp/tools_notes.go goes back to os.WriteFile(target.Abs, ...), go test ./internal/mcp ./internal/notes ./internal/brainfs stays green. I put a seam before the stamp, swapped the note for a symlink, and forced a stamp failure. Under the PR the rollback refuses the swap, which is correct. With the revert it writes the prior note bytes through the link, to a file outside the brain or another note in it. TestBrainEditRollsBackWhenTheStampCannotBeVerified only covers the case with no swap. Add a test that swaps a link in and goes red on the revert.
The "in-brain" dangling .lock subtest never reaches the new guard.TestBrainEditDoesNotCreateAFileThroughADanglingLockSymlink/in-brain plants an absolute symlink. os.Root refuses every absolute symlink on its own ("path escapes from parent"), so that subtest passes even if the guards are taken out. A relative in-brain dangling link is the shape Syncthing would carry, and os.Root follows it, O_NOFOLLOW or not. Only the Lstat pre-check in OpenLock stops it. Flip: remove that pre-check and the whole suite stays green, while a relative dangling <note>.lock makes brain_edit create a file at the link's target, which is the #192 defect. Make the in-brain case a relative link, or add a case for one.
OpenLock/RemoveLock have no unit tests. Each of these mutations survives the three packages: drop the pre-check, drop the post-open SameFile check, drop the pre-check and SameFile together, drop realDir, drop cleanRel, drop O_NOFOLLOW, drop O_NONBLOCK, drop realDir in RemoveLock. The PR's tests detect only two flips: snapshot back to os.ReadFile (the FIFO test, which has a real positive control) and the lock opened by path again.
Non-blocking (some predate this PR; please file them)
Pre-existing, also on main: flock plus unlink does not serialise processes. Unlock unlinks the path. A peer blocked on the old inode then gets that lock, while the next opener creates a new inode and locks it as well. I measured this with two processes, each running 2000 lock/critical-section/unlock iterations and an O_EXCL marker inside the section. Main overlapped 613–1371 times; the PR overlapped 819–1067. notes.Edit reads inside the lock and renames, so two pods can lose an update and both report success. Removing the flock call entirely leaves the suite green, because no test covers cross-process locking.
New in this PR: spurious lock failures under contention. In the same probe, 16–23 of 2000 OpenLock calls failed with changed while opening: not a regular file. That is a peer's RemoveLock and recreate racing the SameFile check. The message is misleading and the edit fails. One fix covers both 4 and 5: after flock, compare the fd's inode with an Lstat of the path and retry if they differ, or stop unlinking the lock.
The doc comment overclaims.OpenLock says "nothing is created at its target". In the window between the pre-check and the open, a relative dangling link swapped in still gets an empty in-brain file created at its target (the open is refused only afterwards). Name it as the same accepted residual as #167.
Pre-existing: the snapshot is taken outside the lock and the rollback runs after unlock, so a rollback can overwrite a concurrent edit made by another pod.
The branch conflicts with main in brainfs.go after #189, and Gitea reports it not mergeable. Main now has Brain.Remove and RealParent, which do the same job as RemoveLock and the cleanRel+realDir preamble. Review the rebased result again.
What held (executed end to end through the MCP handler with a real .git)
Lock shapes: relative/absolute dangling links (in-brain and outside), live links (in-brain note and outside file), FIFO with and without a writer, socket, directory. Each is refused with an error, with no hang, nothing created and no target modified. A stale regular lock is reused, and a hardlinked lock does not modify its target.
The new tests fail on the base code (FIFO: hang; dangling: file created for both subtests).
go test -race -count=3 passes on mcp, notes and brainfs. CI=1 task check exits 0. CI run #597 (Lint / Test / Vet) is green, but that green does not test any of the findings above.
## Adversarial review (pr-gatekeeper) — BLOCKED
Head 47354dc. Reviewed by execution against a scratch copy; tests mutated one guard at a time. Issue #192 is labelled `risk/high`, so this goes to Mathias for the merge regardless.
### Blocking
1. **The rollback hardening has no assertion.** If `bfs.WriteRegular(target.Rel, prior)` in `internal/mcp/tools_notes.go` goes back to `os.WriteFile(target.Abs, ...)`, `go test ./internal/mcp ./internal/notes ./internal/brainfs` stays green. I put a seam before the stamp, swapped the note for a symlink, and forced a stamp failure. Under the PR the rollback refuses the swap, which is correct. With the revert it writes the prior note bytes through the link, to a file outside the brain or another note in it. `TestBrainEditRollsBackWhenTheStampCannotBeVerified` only covers the case with no swap. Add a test that swaps a link in and goes red on the revert.
2. **The "in-brain" dangling `.lock` subtest never reaches the new guard.** `TestBrainEditDoesNotCreateAFileThroughADanglingLockSymlink/in-brain` plants an absolute symlink. os.Root refuses every absolute symlink on its own ("path escapes from parent"), so that subtest passes even if the guards are taken out. A relative in-brain dangling link is the shape Syncthing would carry, and os.Root follows it, O_NOFOLLOW or not. Only the Lstat pre-check in `OpenLock` stops it. Flip: remove that pre-check and the whole suite stays green, while a relative dangling `<note>.lock` makes brain_edit create a file at the link's target, which is the #192 defect. Make the in-brain case a relative link, or add a case for one.
3. **`OpenLock`/`RemoveLock` have no unit tests.** Each of these mutations survives the three packages: drop the pre-check, drop the post-open SameFile check, drop the pre-check and SameFile together, drop `realDir`, drop `cleanRel`, drop O_NOFOLLOW, drop O_NONBLOCK, drop `realDir` in RemoveLock. The PR's tests detect only two flips: snapshot back to os.ReadFile (the FIFO test, which has a real positive control) and the lock opened by path again.
### Non-blocking (some predate this PR; please file them)
4. **Pre-existing, also on main: flock plus unlink does not serialise processes.** Unlock unlinks the path. A peer blocked on the old inode then gets that lock, while the next opener creates a new inode and locks it as well. I measured this with two processes, each running 2000 lock/critical-section/unlock iterations and an O_EXCL marker inside the section. Main overlapped 613–1371 times; the PR overlapped 819–1067. `notes.Edit` reads inside the lock and renames, so two pods can lose an update and both report success. Removing the flock call entirely leaves the suite green, because no test covers cross-process locking.
5. **New in this PR: spurious lock failures under contention.** In the same probe, 16–23 of 2000 `OpenLock` calls failed with `changed while opening: not a regular file`. That is a peer's RemoveLock and recreate racing the SameFile check. The message is misleading and the edit fails. One fix covers both 4 and 5: after flock, compare the fd's inode with an Lstat of the path and retry if they differ, or stop unlinking the lock.
6. **The doc comment overclaims.** `OpenLock` says "nothing is created at its target". In the window between the pre-check and the open, a relative dangling link swapped in still gets an empty in-brain file created at its target (the open is refused only afterwards). Name it as the same accepted residual as #167.
7. **Pre-existing:** the snapshot is taken outside the lock and the rollback runs after unlock, so a rollback can overwrite a concurrent edit made by another pod.
8. **The branch conflicts with main** in `brainfs.go` after #189, and Gitea reports it not mergeable. Main now has `Brain.Remove` and `RealParent`, which do the same job as `RemoveLock` and the `cleanRel`+`realDir` preamble. Review the rebased result again.
### What held (executed end to end through the MCP handler with a real `.git`)
- Lock shapes: relative/absolute dangling links (in-brain and outside), live links (in-brain note and outside file), FIFO with and without a writer, socket, directory. Each is refused with an error, with no hang, nothing created and no target modified. A stale regular lock is reused, and a hardlinked lock does not modify its target.
- The new tests fail on the base code (FIFO: hang; dangling: file created for both subtests).
- `go test -race -count=3` passes on mcp, notes and brainfs. `CI=1 task check` exits 0. CI run #597 (Lint / Test / Vet) is green, but that green does not test any of the findings above.
brain_edit did its own I/O by path around notes.Edit:
- the pre-edit snapshot used os.ReadFile, which hung on a FIFO note held
open by a writer;
- the rollback (when the provenance stamp fails) used os.WriteFile;
- the edit lock "<note>.lock" was opened with O_CREATE by path, so a
planted dangling .lock symlink made a successful edit create a file at
the link's target, in the brain or outside it.
Snapshot is brainfs ReadRegular, rollback WriteRegular, and the lock is
opened and removed through new brainfs OpenLock/RemoveLock: clean path,
real parent directories, an existing lock must be regular, and the opened
handle must be the file at the path (SameFile).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 1 on PR #194:
- The edit lock did not exclude other processes (also on main): unlock
removes the lock file, so a waiter could wake holding a flock on the
orphaned inode while the next caller locked a fresh file. flockFile now
re-checks after flock that the locked file is still the one at the path
(brainfs LockIsCurrent) and retries otherwise; OpenLock reports a lock
replaced mid-open as ErrLockChanged, which is retried instead of
failing the edit. New cross-process test (3 re-executed processes x 400
rounds, O_EXCL marker under the lock): 0 overlaps; red without the
re-check.
- OpenLock unit test: relative dangling link (only the Lstat pre-check
stops it; os.Root refuses absolute links on its own), live link, FIFO,
directory, behind a symlinked dir, unclean path, positive control.
- Rollback test: a symlink swapped in for the note before the rollback
(test seam beforeStampHook) is refused, target untouched in and outside
the brain.
- The in-brain dangling-lock case uses a relative link and asserts the
refusal names the lock. OpenLock reuses #189's RealParent; RemoveLock is
dropped for brainfs.Remove. The doc comment names the swap residual.
Rebased onto main after #189.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adversarial review round 2 (pr-gatekeeper) — BLOCKED, one finding
Head 07ea5b0, rebased on main and mergeable. Tested by execution against a scratch copy. Issue #192 is labelled risk/high, so Mathias decides the merge.
Blocking
TestBrainEditRollbackDoesNotWriteThroughASymlinkSwappedInForTheNote passes when the swap never happens.
Flip: I replaced the beforeStampHook() call in tools_notes.go with a no-op. The suite stays green.
Why: without the swap, the rollback writes the real note. The victim stays untouched trivially. The stamp still fails, so resp["error"] is non-nil.
Consequence: the test cannot tell "rollback refused the symlink" apart from "the seam no longer fires". If the hook call is moved after the stamp, or dropped, this becomes decoration with nothing to show it.
Fix: assert the outcome that proves the swap happened and the rollback was refused. Either the error says the edit could NOT be rolled back, or the note path is still a symlink after the call. Either assertion goes red on the no-op flip.
What now holds
Flips that turn a test red:
rollback back to os.WriteFile
OpenLock without the Lstat pre-check (two tests go red)
pre-check and SameFile both removed
no RealParent
lock opened by path again
snapshot back to os.ReadFile
no flock
no LockIsCurrent re-check
LockIsCurrent always true
remove-after-unlock (the old order)
ErrLockChanged treated as fatal
no lock removal
Survive, accepted as race-only guards that the pre-check shadows: the SameFile check in OpenLock alone, O_NONBLOCK alone, and the regular check on the opened handle alone.
Cross-process exclusion, with my own probe (re-executed processes, 2000 rounds each, O_EXCL marker under the lock):
2 processes and 4 processes both give 0 overlaps and 0 lock errors, in two runs each.
Round 1 on main gave hundreds of overlaps; the PR's previous head gave about 1% spurious lock errors. Both are fixed.
Lock shapes through the MCP handler with a real .git:
Relative and absolute dangling links (in-brain and outside), live links, a FIFO with and without a writer, a socket and a directory are all refused, with the error naming the lock.
No hang, nothing created, no target modified.
A stale regular lock is reused, and a hardlinked lock leaves its target unchanged.
Checks:go test -race -count=3 passes on mcp, notes and brainfs. CI=1 task check exits 0. CI run #604 (task 12728) is green.
Non-blocking
The first rollout of this change does not get exclusion. Running the old code against the new code in the probe still overlaps: 18–357 overlaps per old process and 575–1137 per new process, in 2000 rounds. The old pod unlinks after unlocking, and new code cannot defend against that. During the RollingUpdate that ships this PR, two pods editing the same note can still lose an update. From the rollout after that, the lock holds. This is worth a line in the PR description or an issue; no code change is needed.
The notes package comment says brain-fix and brain-enrich "write the same tree". Nothing else takes this lock: notes.Edit is the only flock user. That predates this PR and is outside #192.
## Adversarial review round 2 (pr-gatekeeper) — BLOCKED, one finding
Head 07ea5b0, rebased on main and mergeable. Tested by execution against a scratch copy. Issue #192 is labelled `risk/high`, so Mathias decides the merge.
### Blocking
1. **`TestBrainEditRollbackDoesNotWriteThroughASymlinkSwappedInForTheNote` passes when the swap never happens.**
- **Flip:** I replaced the `beforeStampHook()` call in `tools_notes.go` with a no-op. The suite stays green.
- **Why:** without the swap, the rollback writes the real note. The victim stays untouched trivially. The stamp still fails, so `resp["error"]` is non-nil.
- **Consequence:** the test cannot tell "rollback refused the symlink" apart from "the seam no longer fires". If the hook call is moved after the stamp, or dropped, this becomes decoration with nothing to show it.
- **Fix:** assert the outcome that proves the swap happened and the rollback was refused. Either the error says the edit could NOT be rolled back, or the note path is still a symlink after the call. Either assertion goes red on the no-op flip.
### What now holds
- **Flips that turn a test red:**
- rollback back to `os.WriteFile`
- OpenLock without the Lstat pre-check (two tests go red)
- pre-check and SameFile both removed
- no `RealParent`
- lock opened by path again
- snapshot back to `os.ReadFile`
- no flock
- no `LockIsCurrent` re-check
- `LockIsCurrent` always true
- remove-after-unlock (the old order)
- `ErrLockChanged` treated as fatal
- no lock removal
- **Survive, accepted as race-only guards that the pre-check shadows:** the SameFile check in OpenLock alone, O_NONBLOCK alone, and the regular check on the opened handle alone.
- **Cross-process exclusion, with my own probe** (re-executed processes, 2000 rounds each, O_EXCL marker under the lock):
- 2 processes and 4 processes both give 0 overlaps and 0 lock errors, in two runs each.
- Round 1 on main gave hundreds of overlaps; the PR's previous head gave about 1% spurious lock errors. Both are fixed.
- **Lock shapes through the MCP handler with a real `.git`:**
- Relative and absolute dangling links (in-brain and outside), live links, a FIFO with and without a writer, a socket and a directory are all refused, with the error naming the lock.
- No hang, nothing created, no target modified.
- A stale regular lock is reused, and a hardlinked lock leaves its target unchanged.
- **Checks:** `go test -race -count=3` passes on mcp, notes and brainfs. `CI=1 task check` exits 0. CI run #604 (task 12728) is green.
### Non-blocking
2. **The first rollout of this change does not get exclusion.** Running the old code against the new code in the probe still overlaps: 18–357 overlaps per old process and 575–1137 per new process, in 2000 rounds. The old pod unlinks after unlocking, and new code cannot defend against that. During the RollingUpdate that ships this PR, two pods editing the same note can still lose an update. From the rollout after that, the lock holds. This is worth a line in the PR description or an issue; no code change is needed.
3. The `notes` package comment says brain-fix and brain-enrich "write the same tree". Nothing else takes this lock: `notes.Edit` is the only flock user. That predates this PR and is outside #192.
Review round 2 on PR #194: with the hook made a no-op the test still
passed (no swap, the rollback wrote the real note, victim trivially
untouched). It now asserts the error says the edit could NOT be rolled
back and that the note path is still the swapped-in symlink; red when
the hook does not fire.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Head cdcff12. The only change since 07ea5b0 is six lines in edit_pathio_test.go; no production code changed. I tested by execution against a scratch copy.
Round-2 blocker is fixed. The rollback test now asserts that the error says the edit could NOT be rolled back, and that the note path is still the symlink planted before the stamp.
Turning the beforeStampHook() call into a no-op now fails this test, and only this test.
Moving the hook call to just before the rollback write keeps the test green. That is correct: the swap still happens before the rollback.
Flips re-run at this head, all red:
rollback back to os.WriteFile
rollback skipped entirely: this test and TestBrainEditRollsBackWhenTheStampCannotBeVerified
no OpenLock pre-check: the dangling-lock test and TestOpenLock...
no LockIsCurrent re-check: the cross-process test
Gates:
go test -race -count=3 passes on mcp, notes and brainfs.
CI=1 task check exits 0, with 0 --- FAIL lines.
CI run #608 (task 12737, Lint / Test / Vet) is green and the PR is mergeable.
Carried over from round 2. The production code is identical to 07ea5b0, so I did not re-run these:
0 overlaps across 2 and 4 processes
every lock-file shape refused through the MCP handler
#197: other writers do not take the lock, and the deploy of this PR runs an old pod and a new pod side by side, which still overlap.
risk/high: merging deploys ingestion, so Mathias decides the merge.
## Adversarial review round 3 (pr-gatekeeper): PASS
Head cdcff12. The only change since 07ea5b0 is six lines in `edit_pathio_test.go`; no production code changed. I tested by execution against a scratch copy.
**Round-2 blocker is fixed.** The rollback test now asserts that the error says the edit could NOT be rolled back, and that the note path is still the symlink planted before the stamp.
- Turning the `beforeStampHook()` call into a no-op now fails this test, and only this test.
- Moving the hook call to just before the rollback write keeps the test green. That is correct: the swap still happens before the rollback.
**Flips re-run at this head, all red:**
- rollback back to `os.WriteFile`
- rollback skipped entirely: this test and `TestBrainEditRollsBackWhenTheStampCannotBeVerified`
- no OpenLock pre-check: the dangling-lock test and `TestOpenLock...`
- no `LockIsCurrent` re-check: the cross-process test
**Gates:**
- `go test -race -count=3` passes on mcp, notes and brainfs.
- `CI=1 task check` exits 0, with 0 `--- FAIL` lines.
- CI run #608 (task 12737, Lint / Test / Vet) is green and the PR is mergeable.
**Carried over from round 2.** The production code is identical to 07ea5b0, so I did not re-run these:
- 0 overlaps across 2 and 4 processes
- every lock-file shape refused through the MCP handler
**Tracked elsewhere:**
- #196: the snapshot is read outside the lock.
- #197: other writers do not take the lock, and the deploy of this PR runs an old pod and a new pod side by side, which still overlap.
`risk/high`: merging deploys ingestion, so Mathias decides the merge.
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 #192. risk/high: control code, adversarial review required.
Problem
brain_editdid its own I/O by path aroundnotes.Edit:os.ReadFile, which hung on a FIFO note held open by a writer.os.WriteFile, by path.<note>.lockwas opened withO_CREATEby path. A planted dangling.locksymlink made a successful edit create a file at the link's target, outside the brain or elsewhere in it.Change
brainfs.ReadRegular, and the rollback usesbrainfs.WriteRegular.brainfs.OpenLock/RemoveLock:O_NOFOLLOW|O_NONBLOCK, then confirmed withSameFile. os.Root follows an in-root final symlink even withO_NOFOLLOW.Tests (red before the fix)
TestBrainEditReturnsPromptlyOnAFIFONoteWithAWriter, with a positive control.TestBrainEditDoesNotCreateAFileThroughADanglingLockSymlink: outside and in-brain targets.Flips (red)
os.ReadFile🤖 Generated with Claude Code
Created via git-mcp on behalf of @mathias
Adversarial review (pr-gatekeeper) — BLOCKED
Head
47354dc. Reviewed by execution against a scratch copy; tests mutated one guard at a time. Issue #192 is labelledrisk/high, so this goes to Mathias for the merge regardless.Blocking
The rollback hardening has no assertion. If
bfs.WriteRegular(target.Rel, prior)ininternal/mcp/tools_notes.gogoes back toos.WriteFile(target.Abs, ...),go test ./internal/mcp ./internal/notes ./internal/brainfsstays green. I put a seam before the stamp, swapped the note for a symlink, and forced a stamp failure. Under the PR the rollback refuses the swap, which is correct. With the revert it writes the prior note bytes through the link, to a file outside the brain or another note in it.TestBrainEditRollsBackWhenTheStampCannotBeVerifiedonly covers the case with no swap. Add a test that swaps a link in and goes red on the revert.The "in-brain" dangling
.locksubtest never reaches the new guard.TestBrainEditDoesNotCreateAFileThroughADanglingLockSymlink/in-brainplants an absolute symlink. os.Root refuses every absolute symlink on its own ("path escapes from parent"), so that subtest passes even if the guards are taken out. A relative in-brain dangling link is the shape Syncthing would carry, and os.Root follows it, O_NOFOLLOW or not. Only the Lstat pre-check inOpenLockstops it. Flip: remove that pre-check and the whole suite stays green, while a relative dangling<note>.lockmakes brain_edit create a file at the link's target, which is the #192 defect. Make the in-brain case a relative link, or add a case for one.OpenLock/RemoveLockhave no unit tests. Each of these mutations survives the three packages: drop the pre-check, drop the post-open SameFile check, drop the pre-check and SameFile together, droprealDir, dropcleanRel, drop O_NOFOLLOW, drop O_NONBLOCK, droprealDirin RemoveLock. The PR's tests detect only two flips: snapshot back to os.ReadFile (the FIFO test, which has a real positive control) and the lock opened by path again.Non-blocking (some predate this PR; please file them)
notes.Editreads inside the lock and renames, so two pods can lose an update and both report success. Removing the flock call entirely leaves the suite green, because no test covers cross-process locking.OpenLockcalls failed withchanged while opening: not a regular file. That is a peer's RemoveLock and recreate racing the SameFile check. The message is misleading and the edit fails. One fix covers both 4 and 5: after flock, compare the fd's inode with an Lstat of the path and retry if they differ, or stop unlinking the lock.OpenLocksays "nothing is created at its target". In the window between the pre-check and the open, a relative dangling link swapped in still gets an empty in-brain file created at its target (the open is refused only afterwards). Name it as the same accepted residual as #167.brainfs.goafter #189, and Gitea reports it not mergeable. Main now hasBrain.RemoveandRealParent, which do the same job asRemoveLockand thecleanRel+realDirpreamble. Review the rebased result again.What held (executed end to end through the MCP handler with a real
.git)go test -race -count=3passes on mcp, notes and brainfs.CI=1 task checkexits 0. CI run #597 (Lint / Test / Vet) is green, but that green does not test any of the findings above.47354dc4fcto07ea5b01ecAdversarial review round 2 (pr-gatekeeper) — BLOCKED, one finding
Head
07ea5b0, rebased on main and mergeable. Tested by execution against a scratch copy. Issue #192 is labelledrisk/high, so Mathias decides the merge.Blocking
TestBrainEditRollbackDoesNotWriteThroughASymlinkSwappedInForTheNotepasses when the swap never happens.beforeStampHook()call intools_notes.gowith a no-op. The suite stays green.resp["error"]is non-nil.What now holds
os.WriteFileRealParentos.ReadFileLockIsCurrentre-checkLockIsCurrentalways trueErrLockChangedtreated as fatal.git:go test -race -count=3passes on mcp, notes and brainfs.CI=1 task checkexits 0. CI run #604 (task 12728) is green.Non-blocking
notespackage comment says brain-fix and brain-enrich "write the same tree". Nothing else takes this lock:notes.Editis the only flock user. That predates this PR and is outside #192.Adversarial review round 3 (pr-gatekeeper): PASS
Head
cdcff12. The only change since07ea5b0is six lines inedit_pathio_test.go; no production code changed. I tested by execution against a scratch copy.Round-2 blocker is fixed. The rollback test now asserts that the error says the edit could NOT be rolled back, and that the note path is still the symlink planted before the stamp.
beforeStampHook()call into a no-op now fails this test, and only this test.Flips re-run at this head, all red:
os.WriteFileTestBrainEditRollsBackWhenTheStampCannotBeVerifiedTestOpenLock...LockIsCurrentre-check: the cross-process testGates:
go test -race -count=3passes on mcp, notes and brainfs.CI=1 task checkexits 0, with 0--- FAILlines.Carried over from round 2. The production code is identical to
07ea5b0, so I did not re-run these:Tracked elsewhere:
risk/high: merging deploys ingestion, so Mathias decides the merge.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.