From a5f502eb9c4bbe13b6720ccaf930d41f265a2d16 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 08:18:30 +0000 Subject: [PATCH] fix(api): UpdateNote reserves updated_at/supersedes/supersede_reason from the caller's fragment (#137) Closes #125. Skips updated_at/supersedes/supersede_reason in the caller fragment merge loop so they cannot be set via the fragment, only via the intended entry points. delete_branch_after_merge=true Co-authored-by: Mathias --- ingestion/internal/api/update.go | 17 ++++++++++-- ingestion/internal/api/update_test.go | 39 +++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/ingestion/internal/api/update.go b/ingestion/internal/api/update.go index 69f4d25..085796d 100644 --- a/ingestion/internal/api/update.go +++ b/ingestion/internal/api/update.go @@ -110,9 +110,22 @@ func UpdateNote(brainDir string, opts UpdateNoteOptions) (relPath, contentHash, fm, _ := parseFrontmatter(string(prior)) for _, l := range contentFM.lines { - if l.key != "" { - fm.set(l.key, l.value) + // updated_at/supersedes/supersede_reason are this function's own + // output, not something a caller's fragment sets — updated_at and + // supersedes are already unconditionally re-set below regardless of + // what the loop does with them, but supersede_reason is only + // re-set when opts.Reason is non-empty, so without this skip a + // caller could set it via its own fragment and have it survive + // even when Reason is empty, bypassing the one intended entry + // point for that field (hyperguild#125, found in #111's review). + // Skipping all three here rather than relying on the unconditional + // overwrites below to save the other two keeps the invariant + // explicit at the one place it can be silently broken by a future + // reordering, not implicit in call order. + if l.key == "" || l.key == "updated_at" || l.key == "supersedes" || l.key == "supersede_reason" { + continue } + fm.set(l.key, l.value) } fm.set("updated_at", time.Now().UTC().Format(time.RFC3339)) fm.set("supersedes", priorHash) diff --git a/ingestion/internal/api/update_test.go b/ingestion/internal/api/update_test.go index 85707f7..385c7f1 100644 --- a/ingestion/internal/api/update_test.go +++ b/ingestion/internal/api/update_test.go @@ -231,6 +231,45 @@ func TestUpdateThenReadRoundTripsHash(t *testing.T) { assert.Equal(t, hash, readHash, "update content_hash must round-trip through ReadNote") } +func TestUpdateNoteSupersedeReasonNotSettableViaCallerFragmentWhenReasonOptionEmpty(t *testing.T) { + // hyperguild#125, found in #111's review: the merge loop applied every + // field from the caller's leading frontmatter fragment, including + // supersede_reason -- which is only re-set unconditionally when + // opts.Reason is non-empty, so a caller could set it via its own + // fragment and have it survive even with Reason left empty, bypassing + // the one intended entry point for that field. + brainDir := t.TempDir() + rel := seedNote(t, brainDir, "wiki/a/facts/n.md", "---\nwing: a\nhall: facts\n---\nold\n") + + _, _, _, err := UpdateNote(brainDir, UpdateNoteOptions{ + Path: rel, + Content: "---\nsupersede_reason: snuck in via the fragment\n---\nnew\n", + // Reason deliberately left empty. + }) + require.NoError(t, err) + + got, err := os.ReadFile(filepath.Join(brainDir, filepath.FromSlash(rel))) + require.NoError(t, err) + assert.NotContains(t, string(got), "supersede_reason", + "supersede_reason must not appear at all when opts.Reason is empty, regardless of what the fragment tried to set") +} + +func TestUpdateNoteSupersedeReasonFromOptionStillWorks(t *testing.T) { + // Regression pin: the legitimate entry point (opts.Reason) must be + // completely unaffected by #125's fix. + brainDir := t.TempDir() + rel := seedNote(t, brainDir, "wiki/a/facts/n.md", "---\nwing: a\nhall: facts\n---\nold\n") + + _, _, _, err := UpdateNote(brainDir, UpdateNoteOptions{ + Path: rel, Content: "new\n", Reason: "facts changed", + }) + require.NoError(t, err) + + got, err := os.ReadFile(filepath.Join(brainDir, filepath.FromSlash(rel))) + require.NoError(t, err) + assert.Contains(t, string(got), "supersede_reason: facts changed") +} + func TestContentHashStable(t *testing.T) { assert.Equal(t, ContentHash([]byte("abc")), ContentHash([]byte("abc"))) assert.NotEqual(t, ContentHash([]byte("abc")), ContentHash([]byte("abd")))