From 5a5d938e8e1d0f9d356e9d0d1d2bfed80dc506ad Mon Sep 17 00:00:00 2001 From: Mathias Date: Tue, 22 Sep 2026 21:27:11 +0000 Subject: [PATCH] Brain-quality sweep, batch 1: brain_edit/brain_update correctness + close-session docs (#124) Co-authored-by: Mathias --- ingestion/internal/api/update.go | 36 ++++++- ingestion/internal/api/update_test.go | 108 +++++++++++++++++++++ ingestion/internal/mcp/tools_notes.go | 8 ++ ingestion/internal/mcp/tools_notes_test.go | 23 +++++ skills/close-session/SKILL.md | 3 +- 5 files changed, 176 insertions(+), 2 deletions(-) diff --git a/ingestion/internal/api/update.go b/ingestion/internal/api/update.go index e493006..69f4d25 100644 --- a/ingestion/internal/api/update.go +++ b/ingestion/internal/api/update.go @@ -10,6 +10,7 @@ import ( "time" "github.com/mathiasbq/hyperguild/ingestion/internal/brain" + "github.com/mathiasbq/hyperguild/ingestion/internal/claim" ) // ContentHash returns the lowercase hex sha256 of b. It is the note's @@ -98,14 +99,47 @@ func UpdateNote(brainDir string, opts UpdateNoteOptions) (relPath, contentHash, } priorHash = ContentHash(prior) + // A caller may lead opts.Content with its own frontmatter fence to set or + // change fields on supersede (e.g. the claim envelope). Split it off + // before it reaches the body — left un-split, it would render as a + // second, inert `---`-fenced block of literal text rather than taking + // effect, and any claim-envelope fields inside it would reach disk + // unvalidated (hyperguild#111: the same rule that guards note birth in + // writeHallNote must guard a change to the same fields). + contentFM, contentBody := parseFrontmatter(opts.Content) + fm, _ := parseFrontmatter(string(prior)) + for _, l := range contentFM.lines { + if l.key != "" { + fm.set(l.key, l.value) + } + } fm.set("updated_at", time.Now().UTC().Format(time.RFC3339)) fm.set("supersedes", priorHash) if opts.Reason != "" { fm.set("supersede_reason", opts.Reason) } - out := []byte(fm.render() + opts.Content) + // Validate the envelope AFTER the merge, against the full field set that + // is about to reach disk — not just the caller's fragment. Validating the + // fragment alone would miss a caller that supersedes claim_provenance to + // empty while half_life/applies survive untouched from the prior note: + // each side looks individually fine, but the merged result is an + // unprovenanced envelope, which is the exact laundering this check + // exists to catch. (Caught in review before merge, not filed after — + // worth being honest about in the history: this second pass was the + // actual fix; the first one validated the wrong document.) + envelope := make(map[string]string, len(fm.lines)) + for _, l := range fm.lines { + if l.key != "" { + envelope[l.key] = l.value + } + } + if err := claim.ValidateEnvelope(envelope); err != nil { + return "", "", "", fmt.Errorf("claim envelope: %w", err) + } + + out := []byte(fm.render() + contentBody) if err := os.WriteFile(abs, out, 0o644); err != nil { return "", "", "", fmt.Errorf("write: %w", err) } diff --git a/ingestion/internal/api/update_test.go b/ingestion/internal/api/update_test.go index d677a29..85707f7 100644 --- a/ingestion/internal/api/update_test.go +++ b/ingestion/internal/api/update_test.go @@ -52,6 +52,114 @@ func TestUpdateNoteSupersedesAndStamps(t *testing.T) { assert.Contains(t, s, "supersede_reason: facts changed") } +func TestUpdateNoteRejectsInvalidEnvelopeAndWritesNothing(t *testing.T) { + // hyperguild#111: WriteNote validates the claim envelope before create; + // UpdateNote didn't before supersede, so the same rule must hold on + // change as on birth, fail-closed, nothing written on rejection. + brainDir := t.TempDir() + original := "---\nwing: jepa-fx\nhall: facts\ncreated_at: 2026-01-01T00:00:00Z\n---\n# Old\n\nold body\n" + rel := seedNote(t, brainDir, "wiki/jepa-fx/facts/val-vol.md", original) + + _, _, _, err := UpdateNote(brainDir, UpdateNoteOptions{ + Path: rel, + Content: "---\nhalf_life: forever\n---\n# New\n\nnew body\n", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "claim envelope") + + got, err := os.ReadFile(filepath.Join(brainDir, filepath.FromSlash(rel))) + require.NoError(t, err) + assert.Equal(t, original, string(got), "rejected update must leave the note byte-identical") +} + +func TestUpdateNoteRejectsAppliesWithoutProvenance(t *testing.T) { + brainDir := t.TempDir() + original := "---\nwing: jepa-fx\nhall: facts\n---\nold\n" + rel := seedNote(t, brainDir, "wiki/jepa-fx/facts/val-vol.md", original) + + _, _, _, err := UpdateNote(brainDir, UpdateNoteOptions{ + Path: rel, + Content: "---\napplies: universal\n---\nnew\n", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "claim envelope") + + got, err := os.ReadFile(filepath.Join(brainDir, filepath.FromSlash(rel))) + require.NoError(t, err) + assert.Equal(t, original, string(got)) +} + +func TestUpdateNoteAcceptsValidEnvelopeAndMergesIntoFrontmatter(t *testing.T) { + brainDir := t.TempDir() + rel := seedNote(t, brainDir, "wiki/jepa-fx/facts/val-vol.md", + "---\nwing: jepa-fx\nhall: facts\ncreated_at: 2026-01-01T00:00:00Z\n---\nold\n") + + _, _, _, err := UpdateNote(brainDir, UpdateNoteOptions{ + Path: rel, + Content: "---\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\nnew body\n", + }) + require.NoError(t, err) + + got, err := os.ReadFile(filepath.Join(brainDir, filepath.FromSlash(rel))) + require.NoError(t, err) + s := string(got) + assert.Contains(t, s, "half_life: durable") + assert.Contains(t, s, "applies: universal") + assert.Contains(t, s, "claim_provenance: asserted") + assert.Contains(t, s, "wing: jepa-fx", "prior fields survive") + assert.Contains(t, s, "new body") + assert.Equal(t, 2, strings.Count(s, "---\n"), + "exactly one open+close fence pair — the caller's own fence must not leak in as a second, inert block") +} + +func TestUpdateNoteRejectsClearingProvenanceWhileEnvelopeSurvives(t *testing.T) { + // Found in review, not written speculatively: validating only the + // caller's fragment (rather than the merged result) let a caller blank + // out claim_provenance alone while half_life/applies survived from the + // prior note untouched — each side looks fine on its own, the merged + // envelope on disk is the unprovenanced one the whole check exists to + // catch. + brainDir := t.TempDir() + original := "---\nwing: jepa-fx\nhall: facts\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\nold\n" + rel := seedNote(t, brainDir, "wiki/jepa-fx/facts/val-vol.md", original) + + _, _, _, err := UpdateNote(brainDir, UpdateNoteOptions{ + Path: rel, + Content: "---\nclaim_provenance: \n---\nnew body\n", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "claim envelope") + + got, err := os.ReadFile(filepath.Join(brainDir, filepath.FromSlash(rel))) + require.NoError(t, err) + assert.Equal(t, original, string(got), "rejected update must leave the note byte-identical") +} + +func TestUpdateNoteWithoutEnvelopeFieldsIsUnchangedFromToday(t *testing.T) { + // Regression pin: plain fence-less content (the common case) must behave + // exactly as it did before the envelope-merge fix. + brainDir := t.TempDir() + rel := seedNote(t, brainDir, "wiki/jepa-fx/facts/val-vol.md", + "---\nwing: jepa-fx\nhall: facts\ncreated_at: 2026-01-01T00:00:00Z\ncustom: keep-me\n---\n# Old\n\nold body\n") + + relPath, hash, priorHash, err := UpdateNote(brainDir, UpdateNoteOptions{ + Path: rel, + Content: "# New\n\nnew body\n", + Reason: "facts changed", + }) + require.NoError(t, err) + assert.Equal(t, rel, relPath) + assert.NotEmpty(t, hash) + assert.NotEqual(t, hash, priorHash) + + got, err := os.ReadFile(filepath.Join(brainDir, filepath.FromSlash(rel))) + require.NoError(t, err) + s := string(got) + assert.Contains(t, s, "# New") + assert.Contains(t, s, "custom: keep-me") + assert.Contains(t, s, "supersedes: "+priorHash) +} + func TestUpdateNoteResolvesByWingHallSlug(t *testing.T) { brainDir := t.TempDir() seedNote(t, brainDir, "wiki/jepa-fx/facts/val-vol.md", diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index fe0e813..866ffde 100644 --- a/ingestion/internal/mcp/tools_notes.go +++ b/ingestion/internal/mcp/tools_notes.go @@ -3,6 +3,7 @@ package mcp import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "strings" @@ -180,6 +181,13 @@ func (s *Server) editWithScope(args json.RawMessage, scope notes.Scope) (notes.R } res, err := notes.Edit(s.brainDir, a.Path, a.OldStr, a.NewStr, scope) if err != nil { + if errors.Is(err, notes.ErrDegenerate) && a.OldStr == "" { + // hyperguild#115: "old_str is empty" alone doesn't say whether the + // caller sent it empty or a hop between the caller and here lost + // it. Naming the raw argument byte count makes that distinguishable + // without a second investigation. + return notes.Result{}, nil, fmt.Errorf("%w (raw arguments were %d bytes)", err, len(args)) + } return notes.Result{}, nil, err } diff --git a/ingestion/internal/mcp/tools_notes_test.go b/ingestion/internal/mcp/tools_notes_test.go index 540a03d..b936e75 100644 --- a/ingestion/internal/mcp/tools_notes_test.go +++ b/ingestion/internal/mcp/tools_notes_test.go @@ -70,6 +70,29 @@ func TestBrainReadAndEdit_RoundTripThroughJSONRPC(t *testing.T) { "only the matched bytes may change") } +func TestBrainEdit_EmptyOldStrErrorNamesRawArgumentBytes(t *testing.T) { + // hyperguild#115: a session reported "old_str is empty" with an old_str + // that, on their end, was never empty. The server-side path was + // unreachable to reproduce (verified separately: a fresh brain_edit call + // with real content round-trips cleanly), which points at something + // between a specific client and this call losing the field. The fix this + // issue actually asked for isn't a behavior change — old_str=="" must + // still refuse — it's that the refusal should say how many raw argument + // bytes it received, so "client sent empty" is distinguishable from + // "server received empty after some hop lost it" without a second + // session's worth of guessing. + dir := newBrainRepo(t) + srv := mcp.NewServer(dir, nil, nil, nil) + + resp := call(t, srv, "brain_edit", map[string]any{ + "path": "wiki/homelab/facts/n.md", "old_str": "", "new_str": "BETA", + }) + errMsg, ok := resp["error"].(map[string]any)["message"].(string) + require.True(t, ok, "%v", resp) + assert.Contains(t, errMsg, "old_str is empty") + assert.Contains(t, errMsg, "raw arguments were", "%v", errMsg) +} + func TestBrainEdit_RefusesGovernanceFilesFromTheModelFacingTool(t *testing.T) { dir := newBrainRepo(t) srv := mcp.NewServer(dir, nil, nil, nil) diff --git a/skills/close-session/SKILL.md b/skills/close-session/SKILL.md index 4d4f2dc..ef7ad1c 100644 --- a/skills/close-session/SKILL.md +++ b/skills/close-session/SKILL.md @@ -49,12 +49,13 @@ Persist the session via a **single `capture` call** (the `brain:capture` MCP too **Assemble one payload:** - **`insights[]`** — the generalizable learnings from Phase 1 (decisions/failures worth re-reading). Each: `{text, wing, hall}`; add `supersede_slug` to revise a prior note in place instead of creating a duplicate. `hall` ∈ facts/decisions/failures/hypotheses/sources. + Optional claim envelope, per insight: `half_life` ∈ volatile/seasonal/durable, `applies` ∈ local/homelab/universal, `claim_provenance` ∈ asserted/inferred — required together whenever either `half_life` or `applies` is set (an envelope with no provenance is rejected). A close-session insight is usually exactly the durable, generalizable, human-decided kind this describes: `half_life: durable, applies: universal, claim_provenance: asserted` is the common case, not a rare opt-in. - **`tickets[]`** — the issue actions from Phase 3: `{repo, action, ...}` where action ∈ create/close/comment. Owner is always `mathias` (server-forced). - **`context`** — `{harness: "claudeai-chat", session_ref: , fidelity: "live-capture", actor: "mathias", classification: }`. **THE CLASSIFICATION GATE (read before calling — this is where capture refuses).** Capture computes an **effective classification = the strictest across EVERY target it touches** (each insight's `wing`, each ticket's `repo`), then refuses if that effective level is `confidential` and the origin is us-nexus (claude.ai is us-nexus). Levels come from `classification.yaml` at the brain root (source of truth, #67), with the code defaults as the floor: `hyperguild`/`homelab` → internal; `client-*` → confidential; **anything untagged → confidential (fail-safe)**. -- **Tagged `internal` today** (safe through claude.ai): wings `hyperguild`, `homelab`; repos `brain`, `ai-sessions`, `infra`, `hyperguild`, `homelab`, `tapir`, `agentsquad`, `jepa-fx-risk`, `swedsl`. Treat `classification.yaml` as authoritative — this list is a hint, not gospel. +- **The rule, not a list**: untagged → confidential (fail-safe). `classification.yaml` at the brain root is the sole authority on what's tagged `internal` today — it grew from 10 to ~39 repos in one sweep (hyperguild#91) and will keep moving, so read the file rather than trust an enumeration frozen at whatever moment this skill was last edited. - Declare `context.classification: "internal"` for normal homelab work. - Insight `wing`s and ticket `repo`s are classification INPUTS, not free-form metadata — every target must resolve `internal` or the whole capture escalates to `confidential` and the gate refuses via claude.ai. Listing the central homelab repos (incl. `brain`/`ai-sessions`) is now fine; they're tagged. - If a session genuinely touched **`client-*` or otherwise-untagged** material, it cannot be captured through claude.ai — note that in the verdict rather than trying to force it.