fix(mcp): reject edits that strip a note's frontmatter fence
Round-3 review found a fourth bypass: checkClaimEnvelope always validates
frontmatterScalars(after), but an edit whose new_str drops the closing
"---" degrades `after` to "unfenced" in the same call. frontmatterBlock(after)
then returns ok=false, frontmatterScalars returns an empty map, and
claim.ValidateEnvelope({}) is nil by design (absent fields are valid) — so
an invalid enum value lands on disk with zero error, and the note is left
permanently unfenced.
A note that had a valid fence before the edit must still have one after, or
the edit is rejected outright — closes this off structurally rather than
special-casing the fence-removal shape. Also guards oldStr == "" as a no-op
(strings.Replace treats "" as matching position 0, which would otherwise
look like an edit that inserts text before the fence): that degenerate case
belongs to notes.Edit's own "old_str is empty" refusal (#115), not to the
envelope check — TestBrainEdit_EmptyOldStrErrorNamesRawArgumentBytes caught
the regression when this guard was still missing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -269,6 +269,14 @@ func touchesFrontmatter(oldStr, newStr string) bool {
|
|||||||
// the only thing riding on this preview's accuracy is the envelope check
|
// the only thing riding on this preview's accuracy is the envelope check
|
||||||
// itself, not write safety.
|
// itself, not write safety.
|
||||||
func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error {
|
func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error {
|
||||||
|
if oldStr == "" {
|
||||||
|
// hyperguild#115: strings.Replace treats "" as matching position 0,
|
||||||
|
// which would make an empty old_str look like an edit that inserts
|
||||||
|
// text before the fence — a degenerate-input case that belongs to
|
||||||
|
// notes.Edit's own "old_str is empty" refusal, not to the envelope
|
||||||
|
// check.
|
||||||
|
return nil
|
||||||
|
}
|
||||||
before, err := notes.Read(brainDir, path)
|
before, err := notes.Read(brainDir, path)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
// Let notes.Edit itself produce the real error (missing file, etc.)
|
// Let notes.Edit itself produce the real error (missing file, etc.)
|
||||||
@@ -284,7 +292,18 @@ func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error {
|
|||||||
// (and thus the validation outcome) unchanged, so this is a no-op for
|
// (and thus the validation outcome) unchanged, so this is a no-op for
|
||||||
// the common case and fail-closed for every case that touches the
|
// the common case and fail-closed for every case that touches the
|
||||||
// envelope, regardless of how old_str is phrased.
|
// envelope, regardless of how old_str is phrased.
|
||||||
|
_, hadFence := frontmatterBlock(string(before))
|
||||||
after := strings.Replace(string(before), oldStr, newStr, 1)
|
after := strings.Replace(string(before), oldStr, newStr, 1)
|
||||||
|
_, hasFence := frontmatterBlock(after)
|
||||||
|
if hadFence && !hasFence {
|
||||||
|
// Round 3: an edit that drops the closing fence degrades `after`
|
||||||
|
// to "unfenced" — frontmatterScalars then sees an empty map and
|
||||||
|
// ValidateEnvelope({}) is nil by design (absent fields are valid),
|
||||||
|
// so an invalid value would otherwise land with zero error while
|
||||||
|
// also corrupting the note's structure. A previously-fenced note
|
||||||
|
// must stay fenced, full stop.
|
||||||
|
return fmt.Errorf("edit would remove the frontmatter fence")
|
||||||
|
}
|
||||||
return claim.ValidateEnvelope(frontmatterScalars(after))
|
return claim.ValidateEnvelope(frontmatterScalars(after))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -168,6 +168,34 @@ func TestBrainEdit_RejectsAnOldStrThatCrossesTheClosingFence(t *testing.T) {
|
|||||||
assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
|
assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestBrainEdit_RejectsAnEditThatRemovesTheClosingFence(t *testing.T) {
|
||||||
|
// Found in round-3 review: checkClaimEnvelope always validates the
|
||||||
|
// resulting frontmatter of `after` — but an edit whose new_str drops
|
||||||
|
// the closing "---" degrades `after` to "unfenced" in the same call.
|
||||||
|
// frontmatterBlock(after) then returns ok=false, frontmatterScalars
|
||||||
|
// returns an empty map, and claim.ValidateEnvelope({}) is nil by
|
||||||
|
// design (absent fields are valid) — so an invalid enum value lands on
|
||||||
|
// disk with zero error, and the note is left permanently unfenced.
|
||||||
|
// Structural fix: a note that had a valid fence before the edit must
|
||||||
|
// still have one after, or the edit is rejected outright.
|
||||||
|
dir := newBrainRepo(t)
|
||||||
|
note := filepath.Join(dir, "wiki/homelab/facts/n.md")
|
||||||
|
require.NoError(t, os.WriteFile(note,
|
||||||
|
[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
|
||||||
|
srv := mcp.NewServer(dir, nil, nil, nil)
|
||||||
|
|
||||||
|
resp := call(t, srv, "brain_edit", map[string]any{
|
||||||
|
"path": "wiki/homelab/facts/n.md",
|
||||||
|
"old_str": "claim_provenance: asserted\n---\n",
|
||||||
|
"new_str": "claim_provenance: not-a-real-value\n",
|
||||||
|
})
|
||||||
|
require.Contains(t, resp, "error", "%v", resp)
|
||||||
|
|
||||||
|
got, err := os.ReadFile(note)
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.Contains(t, string(got), "claim_provenance: asserted\n---\n", "rejected edit must leave the note byte-identical, fence intact")
|
||||||
|
}
|
||||||
|
|
||||||
func TestBrainEdit_AllowsAValidClaimEnvelopeEdit(t *testing.T) {
|
func TestBrainEdit_AllowsAValidClaimEnvelopeEdit(t *testing.T) {
|
||||||
dir := newBrainRepo(t)
|
dir := newBrainRepo(t)
|
||||||
note := filepath.Join(dir, "wiki/homelab/facts/n.md")
|
note := filepath.Join(dir, "wiki/homelab/facts/n.md")
|
||||||
|
|||||||
Reference in New Issue
Block a user