mathias/hyperguild
1
0
Fork 0
Code Issues 30 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

brain_edit always fails with "old_str is empty" even when old_str is non-empty #115

New Issue
Closed
opened 2026-08-31 05:10:28 +00:00 by mathias · 1 comment
mathias commented 2026-08-31 05:10:28 +00:00
Owner
Copy Link
Copy Source

Symptom

Every brain_edit call this session failed identically:

notes: edit is degenerate: old_str is empty

old_str was never actually empty on my end — tried three times with different content, all failed the same way:

  1. A ~10-line multi-paragraph old_str (verbatim copy from brain_read's own output for the target note).
  2. A 3-line excerpt of the same note.
  3. A trivial one-liner: old_str: "a cold \gemma4:31b` completion"`.

All three targeted the same real note (wiki/homelab/failures/brain-answer-fallback-cold-load-exceeds-shared-timeout-budget.md), all copied exactly from a fresh brain_read of that note (not from a brain_query excerpt — read the tool description, know that rule). All three came back with the identical old_str is empty error.

Where this points in the code

ingestion/internal/notes/edit.go:105:

return Result{}, fmt.Errorf("%w: old_str is empty", ErrDegenerate)

guards oldStr == "" inside Edit(brainDir, path, oldStr, newStr, scope). The caller,
editWithScope in ingestion/internal/mcp/tools_notes.go:176, just does
json.Unmarshal(args, &a) into brainEditArgs{OldStr string \json:"old_str"`}and passesa.OldStrstraight through — nothing server-side looks buggy in isolation, which is why I'm not claiming a root cause here: something between my tool call and this unmarshal is losingold_str`
specifically, and I don't have visibility into that hop from this session.

What this rules out

  • Not a size/multi-line issue — the trivial one-liner failed identically to the 10-line version.
  • Not the wrong-source-text problem the tool description warns about ("never build old_str from a
    brain_query excerpt") — all three attempts used fresh brain_read output, byte-for-byte.
  • Other MCP tools in the same session worked fine throughout, including ones with string args
    (brain_query, brain_write, brain_read on the same note) — so it's not a session-wide
    transport failure, it's specific to brain_edit's old_str argument.

Suggested follow-up

  • Reproduce from a different MCP client than mine (rule out a client-side serialization quirk
    specific to this harness) before assuming server-side.
  • If it does turn out to be server-side: ErrDegenerate's message could echo back what old_str
    length/hash the server actually received, which would have made this a 30-second diagnosis
    instead of a dead end — right now "old_str is empty" gives no way to tell client-sent-empty
    apart from server-received-empty-after-a-marshaling-bug.

Workaround used

Fell back to mcp__brain__capture's supersede_slug on the insights array — that path writes a
whole new note body under the same path/slug rather than patching a byte range, so it didn't hit
whatever brain_edit is hitting.

## Symptom Every `brain_edit` call this session failed identically: ``` notes: edit is degenerate: old_str is empty ``` `old_str` was never actually empty on my end — tried three times with different content, all failed the same way: 1. A ~10-line multi-paragraph `old_str` (verbatim copy from `brain_read`'s own output for the target note). 2. A 3-line excerpt of the same note. 3. A trivial one-liner: `old_str: "a cold \`gemma4:31b\` completion"`. All three targeted the same real note (`wiki/homelab/failures/brain-answer-fallback-cold-load-exceeds-shared-timeout-budget.md`), all copied exactly from a fresh `brain_read` of that note (not from a `brain_query` excerpt — read the tool description, know that rule). All three came back with the identical `old_str is empty` error. ## Where this points in the code `ingestion/internal/notes/edit.go:105`: ```go return Result{}, fmt.Errorf("%w: old_str is empty", ErrDegenerate) ``` guards `oldStr == ""` inside `Edit(brainDir, path, oldStr, newStr, scope)`. The caller, `editWithScope` in `ingestion/internal/mcp/tools_notes.go:176`, just does `json.Unmarshal(args, &a)` into `brainEditArgs{OldStr string \`json:"old_str"\`}` and passes `a.OldStr` straight through — nothing server-side looks buggy in isolation, which is why I'm not claiming a root cause here: something between my tool call and this unmarshal is losing `old_str` specifically, and I don't have visibility into that hop from this session. ## What this rules out - Not a size/multi-line issue — the trivial one-liner failed identically to the 10-line version. - Not the wrong-source-text problem the tool description warns about ("never build old_str from a brain_query excerpt") — all three attempts used fresh `brain_read` output, byte-for-byte. - Other MCP tools in the same session worked fine throughout, including ones with string args (`brain_query`, `brain_write`, `brain_read` on the same note) — so it's not a session-wide transport failure, it's specific to `brain_edit`'s `old_str` argument. ## Suggested follow-up - Reproduce from a different MCP client than mine (rule out a client-side serialization quirk specific to this harness) before assuming server-side. - If it does turn out to be server-side: `ErrDegenerate`'s message could echo back what `old_str` length/hash the server actually received, which would have made this a 30-second diagnosis instead of a dead end — right now "old_str is empty" gives no way to tell client-sent-empty apart from server-received-empty-after-a-marshaling-bug. ## Workaround used Fell back to `mcp__brain__capture`'s `supersede_slug` on the `insights` array — that path writes a whole new note body under the same path/slug rather than patching a byte range, so it didn't hit whatever `brain_edit` is hitting.
mathias added the risk/mediumsize/mtype/fix labels 2026-09-16 22:23:53 +00:00
mathias commented 2026-09-22 21:27:31 +00:00
Author
Owner
Copy Link
Copy Source

Verified and merged in mathias/hyperguild#124, commit 5a5d938e8e.

Second-round fix (e2cb315) builds the claim envelope map from fm (post-merge, pre-write) instead of the caller's raw contentFM fragment, in ingestion/internal/api/update.go. Independently re-verified before merging:

  • Flip-tested TestUpdateNoteRejectsClearingProvenanceWhileEnvelopeSurvives myself: reverted the envelope map to build from contentFM again, confirmed the test goes red ("An error is expected but got nil"); restored the fix, confirmed green.
  • Adversarial case (mine, not in the PR's own tests): prior note with no envelope at all, update sets half_life: durable alone -> correctly rejected, file unchanged.
  • Confirmed updated_at/supersedes cannot be spoofed via the caller's fragment: both are unconditionally re-set after the merge loop.
  • Noted, non-blocking: supersede_reason set via the caller's own fragment survives when opts.Reason is empty -- same-actor two ways to set the same field, not a cross-update provenance leak like the envelope bug, so not treated as blocking.

CI green on the merged head SHA (run 7795, e2cb315, conclusion success).

Verified and merged in mathias/hyperguild#124, commit 5a5d938e8e1d0f9d356e9d0d1d2bfed80dc506ad. Second-round fix (e2cb315) builds the claim envelope map from `fm` (post-merge, pre-write) instead of the caller's raw `contentFM` fragment, in ingestion/internal/api/update.go. Independently re-verified before merging: - Flip-tested TestUpdateNoteRejectsClearingProvenanceWhileEnvelopeSurvives myself: reverted the envelope map to build from `contentFM` again, confirmed the test goes red ("An error is expected but got nil"); restored the fix, confirmed green. - Adversarial case (mine, not in the PR's own tests): prior note with no envelope at all, update sets `half_life: durable` alone -> correctly rejected, file unchanged. - Confirmed `updated_at`/`supersedes` cannot be spoofed via the caller's fragment: both are unconditionally re-set after the merge loop. - Noted, non-blocking: `supersede_reason` set via the caller's own fragment survives when `opts.Reason` is empty -- same-actor two ways to set the same field, not a cross-update provenance leak like the envelope bug, so not treated as blocking. CI green on the merged head SHA (run 7795, e2cb315, conclusion success).
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
agent/ready
Meets the agent-ready contract; authorizes unattended execution
 blocked/client
Waiting on the client
 blocked/supplier
Waiting on a supplier
 blocked/upstream
Waiting on an upstream project or vendor
 risk/high
Irreversible, security-relevant, or control plane
 risk/low
Reversible, local, test-covered
 risk/medium
Touches shared state, deploys or data
 size/l
Too big - split it
 size/m
Under two days
 size/s
Under half a day
 size/xs
Under an hour
 status:dispatch
status:dispatched
status:review
status:reviewing
type/chore
Maintenance, no behaviour change
 type/decision
Human decision required; never agent/ready
 type/docs
Documentation only
 type/feat
New capability
 type/fix
Corrects broken behaviour
 type/ops
Infrastructure, deploy, runtime
 type/research
Investigate; output is a finding, not code
 type/test
Test coverage only
No labels risk/medium size/m type/fix
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate-bot mathias
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mathias/hyperguild#115