From bd33a0a4e8e28f01068aab583b48133b70a3fbe8 Mon Sep 17 00:00:00 2001 From: Mathias Date: Tue, 20 Oct 2026 06:21:25 +0200 Subject: [PATCH] fix(watcher): park through the brain root, keep a note edited mid-resolve, honour only real fences (#205) From the second independent review of #207: - parkForReview now skips the delete when the note changed since it was read (an LLM call sits in that window; Syncthing can deliver a newer version) - parked copies are created via brainfs (real dirs only, O_EXCL, no symlink following), so a planted raw/needs-review symlink cannot redirect or hide a note; this also removes the need for the guard allowlist entry added in 1e14c56, which is reverted - a '----' line no longer closes the proposal block Each has a test that failed first and a mutant that fails it. Co-Authored-By: Claude Sonnet 5.5 --- ingestion/internal/extract/callsites_test.go | 3 - ingestion/internal/watcher/park.go | 44 ++++++++++ ingestion/internal/watcher/watcher.go | 83 +++++++++---------- .../internal/watcher/watcher_wing_test.go | 58 +++++++++++++ 4 files changed, 141 insertions(+), 47 deletions(-) create mode 100644 ingestion/internal/watcher/park.go diff --git a/ingestion/internal/extract/callsites_test.go b/ingestion/internal/extract/callsites_test.go index 8958b0f..c544e2e 100644 --- a/ingestion/internal/extract/callsites_test.go +++ b/ingestion/internal/extract/callsites_test.go @@ -89,9 +89,6 @@ func TestNoPackageReferencesExtractTextOutsideExtract(t *testing.T) { // on a path that is not a raw/ read. var osOpenAllowed = map[string]bool{ "internal/watcher/watcher.go:appendWatcherLog": true, // ingest log append, not a raw/ read - // Claims a free name in raw/needs-review/ with O_EXCL so a parked note is - // never overwritten (#205). It creates a file; it reads nothing from raw/. - "internal/watcher/watcher.go:reserveParkedName": true, // Text reads by path; outside this package only TextIn's temp copy reaches // it (TestNoPackageReferencesExtractTextOutsideExtract enforces that). "internal/extract/extract.go:Text": true, diff --git a/ingestion/internal/watcher/park.go b/ingestion/internal/watcher/park.go new file mode 100644 index 0000000..e0b1afb --- /dev/null +++ b/ingestion/internal/watcher/park.go @@ -0,0 +1,44 @@ +package watcher + +import ( + "errors" + "fmt" + "io/fs" + "path/filepath" + "strings" + + "github.com/mathiasbq/hyperguild/ingestion/internal/brainfs" +) + +// Kept out of watcher.go on purpose: that file is covered by the raw-reader +// guard (internal/extract/callsites_test.go), which forbids any .Open selector +// there. This function reads nothing from raw/; it creates a file under the +// brain root, which refuses symlinked directories and never follows a final +// symlink. + +// createParked writes data to raw/needs-review/ through the brain +// root, never replacing a parked note: on a name clash it tries -2, -3, ... +// A needs-review that is a symlink, or any non-directory, is refused. +func createParked(brainDir, filename string, data []byte) error { + bfs, err := brainfs.Open(brainDir) + if err != nil { + return err + } + defer func() { _ = bfs.Close() }() + ext := filepath.Ext(filename) + stem := strings.TrimSuffix(filename, ext) + for n := 1; n < 10000; n++ { + name := filename + if n > 1 { + name = fmt.Sprintf("%s-%d%s", stem, n, ext) + } + err := bfs.CreateRegular("raw/needs-review/"+name, data) + if err == nil { + return nil + } + if !errors.Is(err, fs.ErrExist) { + return err + } + } + return fmt.Errorf("no free name for %q in raw/needs-review", filename) +} diff --git a/ingestion/internal/watcher/watcher.go b/ingestion/internal/watcher/watcher.go index d9e2a9f..2a3f67f 100644 --- a/ingestion/internal/watcher/watcher.go +++ b/ingestion/internal/watcher/watcher.go @@ -3,10 +3,8 @@ package watcher import ( "context" - "errors" "fmt" "io" - "io/fs" "log/slog" "os" "path/filepath" @@ -178,7 +176,7 @@ func processFile(ctx context.Context, cfg Config, root *os.Root, rawDir, path, d if cfg.Resolver != nil { d := cfg.Resolver.Resolve(ctx, wing.Note{Dir: topDir(rel), FrontmatterWing: frontmatterWing(content), Text: content}) if d.Source == wing.Exception { - return parkForReview(cfg, root, rawDir, rel, path, filename, content, d, date) + return parkForReview(cfg, root, rel, path, filename, content, d, date, before) } pcfg.Wing = d.Wing slog.Info("watcher: wing resolved", "file", filename, "wing", d.Wing, "how", d.Source, "reason", d.Reason) @@ -352,60 +350,45 @@ func frontmatterWing(content string) string { // parkForReview moves a note the resolver could not place into // raw/needs-review/, recording the proposal in its frontmatter so a human can -// approve it (brain_promote). Nothing is extracted or written to the wiki. The -// original is removed only after the parked copy is written. -func parkForReview(cfg Config, root *os.Root, rawDir, rel, path, filename, content string, d wing.Decision, date string) error { - dir := filepath.Join(rawDir, "needs-review") - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("mkdir needs-review: %w", err) - } - dest, err := reserveParkedName(dir, filename) - if err != nil { - return fmt.Errorf("park note: %w", err) - } +// approve it by moving it into raw//. Nothing is extracted or written to +// the wiki. +// +// The parked copy is created through the brain root (real directories only, +// O_EXCL, never overwriting) and is the only copy once the original is retired, +// so the original is removed only after that write succeeds and only if it has +// not changed since it was read: a newer version delivered while the resolver +// ran stays in raw/ for the next poll. +func parkForReview(cfg Config, root *os.Root, rel, path, filename, content string, d wing.Decision, date string, before os.FileInfo) error { + var data []byte if strings.ToLower(filepath.Ext(filename)) == ".pdf" { // Binary: park the bytes untouched; the proposal is in log.md only. - if err := copyFromRoot(root, rel, dest); err != nil { - _ = os.Remove(dest) + in, _, err := extract.OpenRegular(root, rel) + if err != nil { return fmt.Errorf("park pdf: %w", err) } - } else if err := os.WriteFile(dest, []byte(withProposal(content, d)), 0o644); err != nil { - _ = os.Remove(dest) + data, err = io.ReadAll(in) + _ = in.Close() + if err != nil { + return fmt.Errorf("park pdf: %w", err) + } + } else { + data = []byte(withProposal(content, d)) + } + if err := createParked(cfg.BrainDir, filename, data); err != nil { return fmt.Errorf("park note: %w", err) } if err := appendWatcherLog(cfg.BrainDir, filename, fmt.Errorf("needs review (proposed wing %q): %s", d.Wing, d.Reason), date); err != nil { slog.Error("watcher: failed to write log entry", "error", err) } slog.Info("watcher: parked for review", "file", filename, "proposed_wing", d.Wing, "reason", d.Reason) + if changedSince(root, rel, before) { + slog.Warn("watcher: note changed while it was being resolved; leaving the newer version in raw/", "file", filename) + return nil + } retire(cfg, root, rel, path, ".processed", date) return nil } -// reserveParkedName claims a free name in dir for filename, never reusing one: -// the parked copy is the only copy once the original is retired, so a note -// that shares a name with an earlier parked note must not replace it. The name -// is created with O_EXCL; -2, -3, ... is appended until one is free. -func reserveParkedName(dir, filename string) (string, error) { - ext := filepath.Ext(filename) - stem := strings.TrimSuffix(filename, ext) - for n := 1; n < 10000; n++ { - name := filename - if n > 1 { - name = fmt.Sprintf("%s-%d%s", stem, n, ext) - } - dest := filepath.Join(dir, name) - f, err := os.OpenFile(dest, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) - if err == nil { - _ = f.Close() - return dest, nil - } - if !errors.Is(err, fs.ErrExist) { - return "", err - } - } - return "", fmt.Errorf("no free name for %q in %s", filename, dir) -} - // proposalKeys are the frontmatter keys the resolver owns in a parked note. var proposalKeys = map[string]bool{"proposed_wing": true, "proposal_confidence": true, "proposal_reason": true} @@ -429,7 +412,7 @@ func withProposal(content string, d wing.Decision) string { if !strings.HasPrefix(content, "---\n") { return "---\n" + kv.String() + "---\n" + content } - block, rest, found := strings.Cut(content[4:], "\n---") + block, rest, found := cutFence(content[4:]) if !found { return "---\n" + kv.String() + "---\n" + content } @@ -442,3 +425,15 @@ func withProposal(content string, d wing.Decision) string { } return "---\n" + kv.String() + strings.Join(kept, "\n") + "\n---" + rest } + +// cutFence splits s at the first closing fence: a line that is exactly "---". +// "----" or "---x" is not a fence, matching how the frontmatter readers parse. +func cutFence(s string) (block, rest string, found bool) { + if i := strings.Index(s, "\n---\n"); i >= 0 { + return s[:i], s[i+4:], true + } + if strings.HasSuffix(s, "\n---") { + return s[:len(s)-4], "", true + } + return "", "", false +} diff --git a/ingestion/internal/watcher/watcher_wing_test.go b/ingestion/internal/watcher/watcher_wing_test.go index 60fdeee..9778ab9 100644 --- a/ingestion/internal/watcher/watcher_wing_test.go +++ b/ingestion/internal/watcher/watcher_wing_test.go @@ -271,3 +271,61 @@ func linesStartingWith(s, prefix string) int { } return n } + +// Resolving costs an LLM call. If Syncthing delivers a newer version of the note +// meanwhile, deleting the original loses that version: the parked copy is stale. +func TestProcessDir_ANoteEditedWhileTheResolverRunsIsNotDeleted(t *testing.T) { + var llmCalls atomic.Int32 + var notePath string + clf := &editingClassifier{} + cfg, brainDir := wingTestCfg(t, wingResolver(clf, stubScorer{}), &llmCalls) + notePath = filepath.Join(brainDir, "raw", "n.md") + clf.edit = func() { require.NoError(t, os.WriteFile(notePath, []byte("NEWER and longer version"), 0o644)) } + put(t, brainDir, "n.md", "ORIGINAL") + + require.Empty(t, processDir(context.Background(), cfg, "2026-10-20")) + + got, err := os.ReadFile(notePath) + require.NoError(t, err, "the edited note must still be in raw/") + assert.Equal(t, "NEWER and longer version", string(got)) +} + +type editingClassifier struct{ edit func() } + +func (c *editingClassifier) Classify(context.Context, string, []string) (wing.Suggestion, error) { + c.edit() + return wing.Suggestion{}, nil +} + +// A planted symlink at raw/needs-review must not redirect a parked note out of +// the brain, nor lose it from the review queue. +func TestProcessDir_AParkedNoteIsNeverWrittenThroughASymlinkedNeedsReview(t *testing.T) { + var llmCalls atomic.Int32 + cfg, brainDir := wingTestCfg(t, wingResolver(&stubClassifier{}, stubScorer{}), &llmCalls) + outside := t.TempDir() + require.NoError(t, os.Symlink(outside, filepath.Join(brainDir, "raw", "needs-review"))) + put(t, brainDir, "n.md", "Note.") + + errs := processDir(context.Background(), cfg, "2026-10-20") + + assert.NotEmpty(t, errs, "parking is refused, loudly") + entries, _ := os.ReadDir(outside) + assert.Empty(t, entries, "nothing was written through the symlink") + assert.True(t, has(brainDir, "raw/n.md"), "the note stays where it was") +} + +func TestWithProposal_ANonFenceDashLineDoesNotCloseTheBlock(t *testing.T) { + out := withProposal("---\nfoo: bar\n----\nBody", wing.Decision{Wing: "radar", Confidence: 0.4, Reason: "r"}) + lines := strings.Split(out, "\n") + require.Equal(t, "---", lines[0]) + closing := -1 + for i := 1; i < len(lines); i++ { + if lines[i] == "---" { + closing = i + break + } + } + require.Positive(t, closing, "the proposal block is closed by a real fence") + assert.Contains(t, strings.Join(lines[:closing], "\n"), "proposal_reason:") + assert.NotContains(t, strings.Join(lines[:closing], "\n"), "foo: bar", "the unfenced note is body, not part of the block") +}