fix(api): UpdateNote reserves updated_at/supersedes/supersede_reason from the caller's fragment
CI / Lint / Test / Vet (pull_request) Successful in 32s
CI / Mirror to GitHub (pull_request) Skipped

hyperguild#125, found in #111's review as a non-blocking observation,
decided now rather than left implicit: UpdateNote's merge loop applied
every field from the caller's leading frontmatter fragment onto the
restamped note, including updated_at/supersedes/supersede_reason.
updated_at and supersedes were already safe -- both get unconditionally
re-set right after the loop regardless of what the loop did with them
-- but supersede_reason is only re-set when opts.Reason is non-empty, so
a caller could set it via its own fragment and have it survive even
with the Reason option left empty, bypassing the one intended entry
point for that field.

Decision: these three fields are this function's own output, not
caller-settable, full stop -- skip all three in the merge loop rather
than lean on the other two happening to be safe via call order. Makes
the invariant explicit at the one place it could be silently broken by
a future reordering, instead of implicit in which fm.set calls happen
to run after the loop.

Flip-tested: reverting the skip to just `l.key == ""` lets the caller's
supersede_reason survive with Reason empty -- the new test catches it.
Full module suite green.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-09-23 10:16:24 +02:00
co-authored by Claude Sonnet 5
parent 7ed2a7d3b4
commit fb69f4bf3d
2 changed files with 54 additions and 2 deletions
+15 -2
View File
@@ -110,9 +110,22 @@ func UpdateNote(brainDir string, opts UpdateNoteOptions) (relPath, contentHash,
fm, _ := parseFrontmatter(string(prior)) fm, _ := parseFrontmatter(string(prior))
for _, l := range contentFM.lines { for _, l := range contentFM.lines {
if l.key != "" { // updated_at/supersedes/supersede_reason are this function's own
fm.set(l.key, l.value) // 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("updated_at", time.Now().UTC().Format(time.RFC3339))
fm.set("supersedes", priorHash) fm.set("supersedes", priorHash)
+39
View File
@@ -231,6 +231,45 @@ func TestUpdateThenReadRoundTripsHash(t *testing.T) {
assert.Equal(t, hash, readHash, "update content_hash must round-trip through ReadNote") 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) { func TestContentHashStable(t *testing.T) {
assert.Equal(t, ContentHash([]byte("abc")), ContentHash([]byte("abc"))) assert.Equal(t, ContentHash([]byte("abc")), ContentHash([]byte("abc")))
assert.NotEqual(t, ContentHash([]byte("abc")), ContentHash([]byte("abd"))) assert.NotEqual(t, ContentHash([]byte("abc")), ContentHash([]byte("abd")))