From 5fac39b215f7c19aab5891bc9060ecb2a6d5fd4e Mon Sep 17 00:00:00 2001 From: Luc Date: Fri, 18 Sep 2026 16:40:08 +0000 Subject: [PATCH 1/4] refactor(memory): remove confidence and the refresh source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit confidence was written on every correction from brain trust override, always as "high", and read by nothing — not ranking, not recall, not the MCP tools. A field set to one constant that nothing consumes carries no information. source: refresh was documented as one of three sources and appeared in no code path at all. - Frontmatter parsing ignores unknown keys, so corrections already carrying "confidence:" keep loading; nothing to migrate - Also picks up pre-existing gofmt alignment drift in the edited struct --- internal/memory/memory.go | 31 ++++++++++--------------------- internal/storetest/conformance.go | 16 ++++++---------- internal/trust/engine.go | 13 ++++++------- 3 files changed, 22 insertions(+), 38 deletions(-) diff --git a/internal/memory/memory.go b/internal/memory/memory.go index 1a1aa80..b2327e3 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -30,18 +30,8 @@ func (l Layer) Valid() bool { type Source string const ( - SourceHuman Source = "human" - SourceTool Source = "tool" - SourceRefresh Source = "refresh" -) - -// Confidence represents the certainty level of a memory. -type Confidence string - -const ( - ConfidenceHigh Confidence = "high" - ConfidenceMedium Confidence = "medium" - ConfidenceLow Confidence = "low" + SourceHuman Source = "human" + SourceTool Source = "tool" ) // Memory is the unified type for all five layers. Layer-specific fields are @@ -59,20 +49,19 @@ type Memory struct { Created time.Time `yaml:"created"` // Optional common fields - Updated *time.Time `yaml:"updated,omitempty"` - Source Source `yaml:"source,omitempty"` - Confidence Confidence `yaml:"confidence,omitempty"` - Tags []string `yaml:"tags,omitempty,flow"` - RevisitIf string `yaml:"revisit_if,omitempty"` - Supersedes string `yaml:"supersedes,omitempty"` - Retired bool `yaml:"retired,omitempty"` - RetiredReason string `yaml:"retired_reason,omitempty"` + Updated *time.Time `yaml:"updated,omitempty"` + Source Source `yaml:"source,omitempty"` + Tags []string `yaml:"tags,omitempty,flow"` + RevisitIf string `yaml:"revisit_if,omitempty"` + Supersedes string `yaml:"supersedes,omitempty"` + Retired bool `yaml:"retired,omitempty"` + RetiredReason string `yaml:"retired_reason,omitempty"` // Layer-specific: facts StaleAfter *time.Time `yaml:"stale_after,omitempty"` // Layer-specific: lessons - StreakClean int `yaml:"streak_clean,omitempty"` + StreakClean int `yaml:"streak_clean,omitempty"` RetireAfter int `yaml:"retire_after,omitempty"` // Layer-specific: effectiveness diff --git a/internal/storetest/conformance.go b/internal/storetest/conformance.go index 9a4e778..1da9e08 100644 --- a/internal/storetest/conformance.go +++ b/internal/storetest/conformance.go @@ -21,13 +21,12 @@ func TestStore(t *testing.T, newStore func(t *testing.T) store.Store) { s := newStore(t) m := memory.Memory{ - Layer: memory.LayerFact, - Domain: "database", - Created: time.Date(2026, 4, 10, 0, 0, 0, 0, time.UTC), - Source: memory.SourceHuman, - Confidence: memory.ConfidenceHigh, - Tags: []string{"schema", "performance"}, - Body: "# Users table\n\nThe users table has 12M rows.\n", + Layer: memory.LayerFact, + Domain: "database", + Created: time.Date(2026, 4, 10, 0, 0, 0, 0, time.UTC), + Source: memory.SourceHuman, + Tags: []string{"schema", "performance"}, + Body: "# Users table\n\nThe users table has 12M rows.\n", } path, err := s.Write(ctx, m) @@ -55,9 +54,6 @@ func TestStore(t *testing.T, newStore func(t *testing.T) store.Store) { if got.Source != m.Source { t.Errorf("Source = %q, want %q", got.Source, m.Source) } - if got.Confidence != m.Confidence { - t.Errorf("Confidence = %q, want %q", got.Confidence, m.Confidence) - } if len(got.Tags) != len(m.Tags) { t.Errorf("Tags = %v, want %v", got.Tags, m.Tags) } else { diff --git a/internal/trust/engine.go b/internal/trust/engine.go index fad74b5..91ba2f8 100644 --- a/internal/trust/engine.go +++ b/internal/trust/engine.go @@ -459,13 +459,12 @@ func (e *Engine) Override(ctx context.Context, domain, reason string) (Decision, } corr := memory.Memory{ - Layer: memory.LayerCorrection, - Domain: domain, - Created: now, - Source: memory.SourceHuman, - Confidence: memory.ConfidenceHigh, - Immutable: true, - Body: reason, + Layer: memory.LayerCorrection, + Domain: domain, + Created: now, + Source: memory.SourceHuman, + Immutable: true, + Body: reason, } if _, err := e.store.Write(ctx, corr); err != nil { return e.decisionFor(domain, d, false), fmt.Errorf("trust: override: state committed but correction write failed: %w", err) From a0f8ec80620a78651741a51d1fa383b5b31d6cea Mon Sep 17 00:00:00 2001 From: Luc Date: Fri, 18 Sep 2026 16:40:16 +0000 Subject: [PATCH 2/4] feat(engine): rank on effectiveness for every domain-scoped recall MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RecallOptions.UseEffectiveness was never set true by anything, so the persona ranking 01-06 built ran in no process brAIn ships. The option is deleted rather than exposed: 01-06 settled that effectiveness surfaces the same way for everyone, and a flag would reopen that. Scores are per-domain, so a recall naming no domain loads nothing and the session-start hook's path is untouched. Two validations ride along, sharing the same test file: - A persona that is not a lower-kebab-case slug is rejected. It would otherwise store fine and then never match a score — a silent failure. - A supersedes path that does not resolve is rejected before anything is written. Previously the memory was written with a warning, leaving the old decision live and the new one claiming to have replaced it: two active decisions contradicting each other over a typo. --- internal/engine/engine.go | 45 +++++++++++++--- internal/engine/engine_test.go | 94 ++++++++++++++++++++++++---------- 2 files changed, 106 insertions(+), 33 deletions(-) diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 72ce773..b7fd382 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -21,11 +21,6 @@ type RecallOptions struct { Layer *memory.Layer // filter to a specific layer (nil = all) Limit int // max results; 0 means no limit IncludeRetired bool // include retired lessons (default false) - - // UseEffectiveness, when true and Domain is set, makes Recall load - // the domain's effectiveness memories and rank memories with higher - // persona acceptance rates above lower ones within the same layer. - UseEffectiveness bool } // RememberResult is the return value of Remember. @@ -125,6 +120,16 @@ func (e *Engine) Remember(ctx context.Context, m memory.Memory) (RememberResult, return RememberResult{}, errors.New("engine: remember: created must not be zero") } + // A persona only earns its keep if it matches the slug Track writes, + // because that is the key effectiveness scores are looked up by + // (ranking.go). An unvalidated persona would be accepted, stored, and + // then silently never score — the quiet failure this pitch exists to + // remove, reintroduced one layer down. + if m.Persona != "" && !isSafeSlug(m.Persona) { + return RememberResult{}, fmt.Errorf( + "%w: persona %q must be lower-kebab-case (e.g. kent-beck)", ErrInvalidArgs, m.Persona) + } + if m.Layer == "" { m.Layer = ClassifyLayer(m.Body) } @@ -141,6 +146,20 @@ func (e *Engine) Remember(ctx context.Context, m memory.Memory) (RememberResult, m.StaleAfter = &stale } + // A supersedes target that does not resolve is rejected before anything + // is written. The alternative, which this replaces, wrote the new memory + // and attached a warning: the superseded decision stayed live and the new + // one claimed to have replaced it, leaving two active decisions that + // contradict each other and a warning on stdout as the only evidence. + // A typo'd path is the likely cause, and a typo should not cost a + // contradiction. + if m.Supersedes != "" { + if _, err := e.store.Read(ctx, m.Supersedes); err != nil { + return RememberResult{}, fmt.Errorf( + "%w: supersedes %q: %w", ErrInvalidArgs, m.Supersedes, err) + } + } + path, err := e.store.Write(ctx, m) if err != nil { return RememberResult{}, fmt.Errorf("engine: remember: %w", err) @@ -148,6 +167,10 @@ func (e *Engine) Remember(ctx context.Context, m memory.Memory) (RememberResult, var warnings []string + // The target resolved a moment ago, so a failure here is a genuine + // write problem rather than a bad path, and the new memory is already + // on disk. A warning is the honest report: nothing to roll back to + // without transactions the markdown store does not have. if m.Supersedes != "" { if err := e.retire(ctx, m.Supersedes, "superseded"); err != nil { warnings = append(warnings, fmt.Sprintf("could not retire superseded memory %q: %v", m.Supersedes, err)) @@ -177,8 +200,18 @@ func (e *Engine) Recall(ctx context.Context, opts RecallOptions) ([]memory.Memor all = filterByQuery(all, opts.Query) } + // Effectiveness ranking is the behaviour, not an option. Scores are + // per-domain, so there is nothing to load for a recall that names no + // domain — which is what the session-start hook runs, leaving its path + // free of this read. Where a domain has no outcomes recorded, the map + // comes back empty and ranking is unchanged. + // + // There is deliberately no flag. Pitch 01-06 settled that effectiveness + // "surfaces in recall results the same way for everyone"; a switch would + // reopen that, and a capability nothing can switch on is what this + // replaced. var scores map[string]float64 - if opts.UseEffectiveness && opts.Domain != "" { + if opts.Domain != "" { scores, err = e.loadEffectivenessScores(ctx, opts.Domain) if err != nil { return nil, fmt.Errorf("engine: recall: %w", err) diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index 8e6d981..ae10034 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -286,26 +286,32 @@ func TestSupersession(t *testing.T) { } } -func TestSupersession_MissingTarget(t *testing.T) { +// Superseding something that is not there is rejected, and nothing is +// written. The previous contract wrote the memory and warned, which left the +// old decision live and the new one claiming to have replaced it — two +// active decisions contradicting each other over a typo. +func TestSupersession_MissingTargetIsRejected(t *testing.T) { e, ctx := setup(t) - now := time.Now() - // Supersede a non-existent file — should succeed with a warning. - res, err := e.Remember(ctx, memory.Memory{ + _, err := e.Remember(ctx, memory.Memory{ Layer: memory.LayerFact, Domain: "db", - Created: now, + Created: time.Now(), Body: "# New fact", Supersedes: "facts/nonexistent.md", }) - if err != nil { - t.Fatalf("Remember should succeed even when superseded target is missing: %v", err) + if !errors.Is(err, engine.ErrInvalidArgs) { + t.Fatalf("err = %v, want ErrInvalidArgs", err) } - if res.Path == "" { - t.Fatal("expected non-empty path") + + // And the rejection must be total: no memory on disk claiming a + // supersession that never happened. + all, err := e.Recall(ctx, engine.RecallOptions{Domain: "db", Limit: 10}) + if err != nil { + t.Fatalf("Recall: %v", err) } - if len(res.Warnings) != 1 { - t.Fatalf("expected 1 warning, got %d", len(res.Warnings)) + if len(all) != 0 { + t.Errorf("rejected remember still wrote %d memories", len(all)) } } @@ -697,27 +703,29 @@ func TestRecall_EffectivenessAdjustsWithinLayer(t *testing.T) { } layer := memory.LayerLesson - // Without the flag: recency wins → hickey first. - plain, err := e.Recall(ctx, engine.RecallOptions{Domain: "testing", Layer: &layer, Limit: 10}) + + // A domain-scoped recall ranks on effectiveness with nothing asked for: + // beck's acceptance rate beats hickey's recency. + scoped, err := e.Recall(ctx, engine.RecallOptions{Domain: "testing", Layer: &layer, Limit: 10}) if err != nil { - t.Fatalf("Recall plain: %v", err) + t.Fatalf("Recall scoped: %v", err) } - if len(plain) != 2 { - t.Fatalf("plain got %d lessons, want 2", len(plain)) + if len(scoped) != 2 { + t.Fatalf("scoped got %d lessons, want 2", len(scoped)) } - if plain[0].Persona != "rich-hickey" { - t.Errorf("plain recall: first lesson = %q, want rich-hickey (newer)", plain[0].Persona) + if scoped[0].Persona != "kent-beck" { + t.Errorf("scoped recall: first lesson = %q, want kent-beck (higher acceptance rate)", scoped[0].Persona) } - // With the flag: beck's higher acceptance rate overrides recency. - adj, err := e.Recall(ctx, engine.RecallOptions{ - Domain: "testing", Layer: &layer, Limit: 10, UseEffectiveness: true, - }) + // Scores are per-domain, so a recall that names no domain has none to + // load and falls back to recency. This is the boundary the session-start + // hook sits on: it recalls across every domain and pays for no lookup. + all, err := e.Recall(ctx, engine.RecallOptions{Layer: &layer, Limit: 10}) if err != nil { - t.Fatalf("Recall adjusted: %v", err) + t.Fatalf("Recall all: %v", err) } - if adj[0].Persona != "kent-beck" { - t.Errorf("adjusted recall: first lesson = %q, want kent-beck (higher rate)", adj[0].Persona) + if all[0].Persona != "rich-hickey" { + t.Errorf("domain-less recall: first lesson = %q, want rich-hickey (newer)", all[0].Persona) } } @@ -725,7 +733,7 @@ func TestRecall_EffectivenessNoOpWithoutData(t *testing.T) { e, ctx := setup(t) // Memory exists but no Track calls → no effectiveness data for the - // domain. The flag must be a no-op, not an error. + // domain. Loading must be a no-op, not an error. mustRemember(t, e, ctx, memory.Memory{ Layer: memory.LayerLesson, Domain: "frontend", @@ -735,7 +743,7 @@ func TestRecall_EffectivenessNoOpWithoutData(t *testing.T) { }) results, err := e.Recall(ctx, engine.RecallOptions{ - Domain: "frontend", UseEffectiveness: true, Limit: 10, + Domain: "frontend", Limit: 10, }) if err != nil { t.Fatalf("Recall: %v", err) @@ -802,3 +810,35 @@ func TestRecall_FullPipeline(t *testing.T) { } } } + +// A persona is the key effectiveness scores are looked up by. One that is +// not a slug would store fine and then never match anything, which is a +// silent failure rather than a loud one. +func TestRemember_RejectsNonSlugPersona(t *testing.T) { + e, ctx := setup(t) + + _, err := e.Remember(ctx, memory.Memory{ + Layer: memory.LayerLesson, + Domain: "testing", + Persona: "Kent Beck", + Created: time.Now(), + Body: "# a lesson", + }) + if !errors.Is(err, engine.ErrInvalidArgs) { + t.Errorf("err = %v, want ErrInvalidArgs", err) + } +} + +// An empty persona is the normal case and must stay allowed. +func TestRemember_AllowsEmptyPersona(t *testing.T) { + e, ctx := setup(t) + + if _, err := e.Remember(ctx, memory.Memory{ + Layer: memory.LayerFact, + Domain: "testing", + Created: time.Now(), + Body: "# a fact with no persona", + }); err != nil { + t.Errorf("Remember: %v", err) + } +} From d26ad04f354f9f9129d5325d379b372a05e74198 Mon Sep 17 00:00:00 2001 From: Luc Date: Fri, 18 Sep 2026 16:40:25 +0000 Subject: [PATCH 3/4] feat(cli): let brain remember set persona, revisit-if and supersedes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fields the data model defines and no interface could write. brain remember set domain, layer and tags; the model defines twelve. --persona is what makes effectiveness ranking mean anything, since nothing else could attribute a memory to a reviewer. - revisit_if surfaces in recall --json, recall text, and the session-start injection. The rules brAIn ships tell a model to reopen a decision when its condition is met, and models read the text — a field the reader cannot see is a field that does not exist. - --supersedes retires the memory it replaces; a path that does not resolve exits 3 with nothing written - An end-to-end test drives the loop through the CLI: track outcomes, attribute memories, watch the overridden persona's newer lesson sink --- internal/cmd/cmd_test.go | 163 +++++++++++++++++++++++++++++++++++++ internal/cmd/hooks.go | 6 +- internal/cmd/recall.go | 20 +++-- internal/cmd/remember.go | 33 ++++++-- internal/cmd/result.go | 13 +-- internal/cmd/track_test.go | 59 ++++++++++++++ 6 files changed, 273 insertions(+), 21 deletions(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 2b54867..aaf4e0b 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -385,3 +385,166 @@ func TestIntegration_FactStalenessComesFromConfig(t *testing.T) { t.Errorf("stale_after = %v, want created+7d = %v", fm.StaleAfter, want) } } + +// A decision is only reopenable if the reader can see what would reopen it. +// The rules brAIn ships tell a model to "reopen one only when its revisit +// condition has actually been met", so the condition has to reach every path +// a consumer reads: the frontmatter, the JSON, and the text. +func TestRemember_revisitIfReachesEveryReader(t *testing.T) { + dir := setupBrainDir(t) + + if code, out := run(t, dir, "remember", "camelCase for API responses", + "--domain", "api", "--layer", "decision", "--revisit-if", "GraphQL adoption"); code != 0 { + t.Fatalf("remember: exit %d, out=%s", code, out) + } + + t.Run("frontmatter", func(t *testing.T) { + matches, err := filepath.Glob(filepath.Join(dir, "decisions", "*.md")) + if err != nil || len(matches) != 1 { + t.Fatalf("want one decision file, got %v (err=%v)", matches, err) + } + raw, err := os.ReadFile(matches[0]) + if err != nil { + t.Fatalf("reading decision: %v", err) + } + if !strings.Contains(string(raw), "revisit_if: GraphQL adoption") { + t.Errorf("revisit_if missing from frontmatter:\n%s", raw) + } + }) + + t.Run("recall --json", func(t *testing.T) { + code, out := run(t, dir, "--json", "recall", "--domain", "api") + if code != 0 { + t.Fatalf("recall: exit %d, out=%s", code, out) + } + var res RecallResult + if err := json.Unmarshal([]byte(out), &res); err != nil { + t.Fatalf("unmarshal: %v (out=%s)", err, out) + } + if len(res.Memories) != 1 || res.Memories[0].RevisitIf != "GraphQL adoption" { + t.Errorf("revisit_if missing from JSON: %+v", res.Memories) + } + }) + + t.Run("recall text", func(t *testing.T) { + code, out := run(t, dir, "recall", "--domain", "api") + if code != 0 { + t.Fatalf("recall: exit %d, out=%s", code, out) + } + if !strings.Contains(out, "revisit if: GraphQL adoption") { + t.Errorf("revisit_if missing from text output:\n%s", out) + } + }) + + t.Run("session-start injection", func(t *testing.T) { + code, out := run(t, dir, "hooks", "session-start") + if code != 0 { + t.Fatalf("session-start: exit %d, out=%s", code, out) + } + if !strings.Contains(out, "revisit if: GraphQL adoption") { + t.Errorf("revisit_if missing from the injected context:\n%s", out) + } + }) +} + +// A memory with no revisit condition must not grow an empty line for it. +func TestRecall_noRevisitLineWhenUnset(t *testing.T) { + dir := setupBrainDir(t) + + if code, out := run(t, dir, "remember", "The users table has 12M rows", + "--domain", "database", "--layer", "fact"); code != 0 { + t.Fatalf("remember: exit %d, out=%s", code, out) + } + code, out := run(t, dir, "recall", "--domain", "database") + if code != 0 { + t.Fatalf("recall: exit %d, out=%s", code, out) + } + if strings.Contains(out, "revisit if:") { + t.Errorf("printed an empty revisit line:\n%s", out) + } +} + +// Superseding is how one decision replaces another. The replaced memory is +// retired, so recall stops surfacing it, and the replacement stands alone. +func TestRemember_supersedesRetiresTheOldDecision(t *testing.T) { + dir := setupBrainDir(t) + + code, out := run(t, dir, "--json", "remember", "snake_case for API responses", + "--domain", "api", "--layer", "decision") + if code != 0 { + t.Fatalf("remember old: exit %d, out=%s", code, out) + } + var first RememberResult + if err := json.Unmarshal([]byte(out), &first); err != nil { + t.Fatalf("unmarshal: %v (out=%s)", err, out) + } + + if code, out := run(t, dir, "remember", "camelCase for API responses", + "--domain", "api", "--layer", "decision", "--supersedes", first.Path); code != 0 { + t.Fatalf("remember new: exit %d, out=%s", code, out) + } + + code, out = run(t, dir, "recall", "--domain", "api") + if code != 0 { + t.Fatalf("recall: exit %d, out=%s", code, out) + } + if !strings.Contains(out, "camelCase") { + t.Errorf("replacement missing from recall:\n%s", out) + } + if strings.Contains(out, "snake_case") { + t.Errorf("superseded decision still surfaces in recall:\n%s", out) + } +} + +// A path that does not resolve is almost always a typo, and a typo must not +// buy two live decisions that contradict each other. +func TestRemember_supersedesRejectsAPathThatIsNotThere(t *testing.T) { + dir := setupBrainDir(t) + + code, _ := run(t, dir, "remember", "camelCase for API responses", + "--domain", "api", "--layer", "decision", "--supersedes", "decisions/typo.md") + if code != 3 { + t.Errorf("exit %d, want 3 (invalid input)", code) + } + + // Nothing may have been written on the way to that rejection. + listCode, listOut := run(t, dir, "list") + if listCode != 0 { + t.Fatalf("list: exit %d", listCode) + } + if !strings.Contains(listOut, "0 memories") { + t.Errorf("a rejected remember left something behind:\n%s", listOut) + } +} + +// confidence was removed from the model. Files already carrying it — every +// correction brain trust override ever wrote — must keep loading, because +// frontmatter parsing ignores keys it does not know. That leniency is what +// made the removal free, and it is worth a test rather than an assumption. +func TestRemovedFields_doNotBreakExistingFiles(t *testing.T) { + dir := setupBrainDir(t) + if err := os.MkdirAll(filepath.Join(dir, "corrections"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + + legacy := "---\n" + + "layer: correction\n" + + "domain: testing\n" + + "created: 2026-04-01T00:00:00Z\n" + + "source: human\n" + + "confidence: high\n" + + "immutable: true\n" + + "---\n" + + "Stop flagging nullable email columns\n" + if err := os.WriteFile(filepath.Join(dir, "corrections", "legacy.md"), []byte(legacy), 0o644); err != nil { + t.Fatalf("writing legacy correction: %v", err) + } + + code, out := run(t, dir, "recall", "--domain", "testing") + if code != 0 { + t.Fatalf("recall: exit %d, out=%s", code, out) + } + if !strings.Contains(out, "Stop flagging nullable email columns") { + t.Errorf("a file carrying the removed confidence field no longer loads:\n%s", out) + } +} diff --git a/internal/cmd/hooks.go b/internal/cmd/hooks.go index efeee29..bb35b3b 100644 --- a/internal/cmd/hooks.go +++ b/internal/cmd/hooks.go @@ -348,7 +348,11 @@ func renderSessionStart(memories []memory.Memory) string { b.WriteString(sessionStartHeader) for _, m := range memories { fmt.Fprintf(&b, "[%s] %s\n", m.Layer, m.Title()) - fmt.Fprintf(&b, " domain: %s path: %s\n\n", m.Domain, m.Path) + fmt.Fprintf(&b, " domain: %s path: %s\n", m.Domain, m.Path) + if m.RevisitIf != "" { + fmt.Fprintf(&b, " revisit if: %s\n", m.RevisitIf) + } + b.WriteString("\n") } b.WriteString(sessionStartFooter) return b.String() diff --git a/internal/cmd/recall.go b/internal/cmd/recall.go index dfaac6b..b8397da 100644 --- a/internal/cmd/recall.go +++ b/internal/cmd/recall.go @@ -50,12 +50,13 @@ func recallCmd() *cobra.Command { jr := RecallResult{} for _, m := range memories { jr.Memories = append(jr.Memories, RecallMemory{ - Path: m.Path, - Layer: string(m.Layer), - Domain: m.Domain, - Title: m.Title(), - Body: m.Body, - Tags: m.Tags, + Path: m.Path, + Layer: string(m.Layer), + Domain: m.Domain, + Title: m.Title(), + Body: m.Body, + Tags: m.Tags, + RevisitIf: m.RevisitIf, }) } @@ -67,6 +68,13 @@ func recallCmd() *cobra.Command { } fmt.Fprintf(&sb, "[%s] %s\n", m.Layer, m.Title()) fmt.Fprintf(&sb, " domain: %s path: %s\n", m.Domain, m.Path) + // A settled decision is only reopenable if the reader can + // see what would reopen it. Printing the condition only in + // --json would hide it from every model that reads the + // text, which is the path the rules point at. + if m.RevisitIf != "" { + fmt.Fprintf(&sb, " revisit if: %s\n", m.RevisitIf) + } } return sb.String() }) diff --git a/internal/cmd/remember.go b/internal/cmd/remember.go index 281c6f4..5c15a7d 100644 --- a/internal/cmd/remember.go +++ b/internal/cmd/remember.go @@ -1,6 +1,7 @@ package cmd import ( + "errors" "fmt" "io" "os" @@ -9,14 +10,18 @@ import ( "github.com/spf13/cobra" + "github.com/luuuc/brain/internal/engine" "github.com/luuuc/brain/internal/memory" ) func rememberCmd() *cobra.Command { var ( - domain string - layer string - tags []string + domain string + layer string + tags []string + persona string + revisitIf string + supersedes string ) cmd := &cobra.Command{ @@ -37,11 +42,14 @@ func rememberCmd() *cobra.Command { } m := memory.Memory{ - Domain: domain, - Body: content, - Created: time.Now(), - Source: memory.SourceHuman, - Tags: tags, + Domain: domain, + Body: content, + Created: time.Now(), + Source: memory.SourceHuman, + Tags: tags, + Persona: persona, + RevisitIf: revisitIf, + Supersedes: supersedes, } if layer != "" { @@ -55,6 +63,9 @@ func rememberCmd() *cobra.Command { eng := engineFrom(cmd) result, err := eng.Remember(cmd.Context(), m) if err != nil { + if errors.Is(err, engine.ErrInvalidArgs) { + return &ExitError{Code: 3, Err: err} + } return err } @@ -79,6 +90,12 @@ func rememberCmd() *cobra.Command { cmd.Flags().StringVar(&domain, "domain", "", "memory domain (required)") cmd.Flags().StringVar(&layer, "layer", "", "memory layer (fact, lesson, decision, effectiveness, correction)") cmd.Flags().StringSliceVar(&tags, "tags", nil, "comma-separated tags") + cmd.Flags().StringVar(&supersedes, "supersedes", "", + "path of the memory this one replaces (e.g. decisions/old-choice.md); it is retired") + cmd.Flags().StringVar(&revisitIf, "revisit-if", "", + "the condition that would make this memory worth reconsidering (e.g. \"GraphQL adoption\")") + cmd.Flags().StringVar(&persona, "persona", "", + "persona this memory came from (e.g. kent-beck); ranks it by that persona's acceptance rate") return cmd } diff --git a/internal/cmd/result.go b/internal/cmd/result.go index ccaa2f3..2e2f318 100644 --- a/internal/cmd/result.go +++ b/internal/cmd/result.go @@ -17,12 +17,13 @@ type RecallResult struct { // RecallMemory is a single memory in recall output. type RecallMemory struct { - Path string `json:"path"` - Layer string `json:"layer"` - Domain string `json:"domain"` - Title string `json:"title"` - Body string `json:"body"` - Tags []string `json:"tags,omitempty"` + Path string `json:"path"` + Layer string `json:"layer"` + Domain string `json:"domain"` + Title string `json:"title"` + Body string `json:"body"` + Tags []string `json:"tags,omitempty"` + RevisitIf string `json:"revisit_if,omitempty"` } // ListResult is the JSON output of brain list. diff --git a/internal/cmd/track_test.go b/internal/cmd/track_test.go index e1085d9..4270204 100644 --- a/internal/cmd/track_test.go +++ b/internal/cmd/track_test.go @@ -167,3 +167,62 @@ func TestTrackIntegration_ViewTextHasRate(t *testing.T) { t.Errorf("view text missing rate:\n%s", out) } } + +// The loop 01-06 built, driven entirely through the CLI: record outcomes for +// two personas, attribute a memory to each, and watch recall rank the +// well-regarded persona's memory above the newer one. +// +// Before --persona existed this was provable only from Go, because nothing +// any user could run put a persona on a lesson. Effectiveness ranking was +// switched on and had nothing to score. +func TestRemember_personaMakesEffectivenessRankingReachable(t *testing.T) { + target := setupBrainDir(t) + + // kent-beck is accepted every time; rich-hickey is overridden every time. + for range 5 { + if code, out := run(t, target, "track", "--domain", "testing", + "--persona", "kent-beck", "--outcome", "accepted"); code != 0 { + t.Fatalf("track beck: exit %d, out=%s", code, out) + } + if code, out := run(t, target, "track", "--domain", "testing", + "--persona", "rich-hickey", "--outcome", "overridden"); code != 0 { + t.Fatalf("track hickey: exit %d, out=%s", code, out) + } + } + + // Beck's lesson is written first, so hickey's is newer and would lead on + // recency alone. + if code, out := run(t, target, "remember", "Check for concurrent writes", + "--domain", "testing", "--layer", "lesson", "--persona", "kent-beck"); code != 0 { + t.Fatalf("remember beck: exit %d, out=%s", code, out) + } + if code, out := run(t, target, "remember", "Prefer values over places", + "--domain", "testing", "--layer", "lesson", "--persona", "rich-hickey"); code != 0 { + t.Fatalf("remember hickey: exit %d, out=%s", code, out) + } + + code, out := run(t, target, "recall", "--domain", "testing", "--layer", "lesson") + if code != 0 { + t.Fatalf("recall: exit %d, out=%s", code, out) + } + beck := strings.Index(out, "Check for concurrent writes") + hickey := strings.Index(out, "Prefer values over places") + if beck < 0 || hickey < 0 { + t.Fatalf("both lessons should be recalled:\n%s", out) + } + if beck > hickey { + t.Errorf("the overridden persona's newer lesson outranked the accepted one:\n%s", out) + } +} + +// A persona that is not a slug would be stored and then never match a score, +// failing silently. It is rejected instead. +func TestRemember_rejectsAPersonaThatCouldNeverScore(t *testing.T) { + target := setupBrainDir(t) + + code, _ := run(t, target, "remember", "A lesson", "--domain", "testing", + "--layer", "lesson", "--persona", "Kent Beck") + if code != 3 { + t.Errorf("exit %d, want 3 (invalid input)", code) + } +} From 101fa86cbd28420d62b1fff029fc1d247aac538b Mon Sep 17 00:00:00 2001 From: Luc Date: Fri, 18 Sep 2026 16:40:25 +0000 Subject: [PATCH 4/4] feat(mcp): give brain_remember the fields the CLI has An AI tool writing through MCP had strictly less reach than a shell: no persona, no revisit condition, no supersession. Anything it cannot set is, for it, a field that does not exist. - brain_remember accepts persona, revisit_if and supersedes; validation is inherited from the engine, so neither interface duplicates the rule - brain_recall returns revisit_if so a tool can read back what it wrote - Corrects the brain_track description, which referenced an option that no longer exists --- internal/mcp/handlers.go | 15 +++++++ internal/mcp/handlers_test.go | 77 +++++++++++++++++++++++++++++++++++ internal/mcp/tools.go | 17 +++++++- 3 files changed, 108 insertions(+), 1 deletion(-) diff --git a/internal/mcp/handlers.go b/internal/mcp/handlers.go index 0b0faa0..956be1b 100644 --- a/internal/mcp/handlers.go +++ b/internal/mcp/handlers.go @@ -70,6 +70,18 @@ func (s *Server) handleRemember(ctx context.Context, args map[string]any) toolCa m.Layer = l } + if persona, ok := args["persona"].(string); ok && persona != "" { + m.Persona = persona + } + + if revisitIf, ok := args["revisit_if"].(string); ok && revisitIf != "" { + m.RevisitIf = revisitIf + } + + if supersedes, ok := args["supersedes"].(string); ok && supersedes != "" { + m.Supersedes = supersedes + } + if tags, ok := args["tags"].(string); ok && tags != "" { for _, t := range strings.Split(tags, ",") { t = strings.TrimSpace(t) @@ -127,6 +139,9 @@ func (s *Server) handleRecall(ctx context.Context, args map[string]any) toolCall "title": m.Title(), "body": m.Body, } + if m.RevisitIf != "" { + item["revisit_if"] = m.RevisitIf + } if len(m.Tags) > 0 { item["tags"] = m.Tags } diff --git a/internal/mcp/handlers_test.go b/internal/mcp/handlers_test.go index 3cf2d9d..342c2bb 100644 --- a/internal/mcp/handlers_test.go +++ b/internal/mcp/handlers_test.go @@ -469,3 +469,80 @@ func containsText(result toolCallResult, substr string) bool { } return strings.Contains(result.Content[0].Text, substr) } + +// An AI tool must have the same reach as a shell: a memory it attributes to +// a persona ranks by that persona's acceptance rate, exactly as one written +// with "brain remember --persona" does. +func TestHandler_Remember_PassesPersonaThrough(t *testing.T) { + var got string + srv := newTestServer(&stubEngine{ + rememberFn: func(_ context.Context, m memory.Memory) (engine.RememberResult, error) { + got = m.Persona + return engine.RememberResult{Path: "lessons/x.md", Layer: memory.LayerLesson}, nil + }, + }) + + res := srv.handleRemember(context.Background(), map[string]any{ + "content": "Check for concurrent writes", + "domain": "payments", + "persona": "kent-beck", + }) + if res.IsError { + t.Fatalf("unexpected error result: %+v", res) + } + if got != "kent-beck" { + t.Errorf("persona reached the engine as %q, want kent-beck", got) + } +} + +// An AI tool writing through MCP must reach the same fields a shell does. +// Anything it cannot set is a field that, for it, does not exist. +func TestHandler_Remember_PassesRevisitIfAndSupersedes(t *testing.T) { + var got memory.Memory + srv := newTestServer(&stubEngine{ + rememberFn: func(_ context.Context, m memory.Memory) (engine.RememberResult, error) { + got = m + return engine.RememberResult{Path: "decisions/x.md", Layer: memory.LayerDecision}, nil + }, + }) + + res := srv.handleRemember(context.Background(), map[string]any{ + "content": "camelCase for API responses", + "domain": "api", + "layer": "decision", + "revisit_if": "GraphQL adoption", + "supersedes": "decisions/snake-case.md", + }) + if res.IsError { + t.Fatalf("unexpected error result: %+v", res) + } + if got.RevisitIf != "GraphQL adoption" { + t.Errorf("revisit_if reached the engine as %q", got.RevisitIf) + } + if got.Supersedes != "decisions/snake-case.md" { + t.Errorf("supersedes reached the engine as %q", got.Supersedes) + } +} + +// And must be able to read back what it wrote. +func TestHandler_Recall_ReturnsRevisitIf(t *testing.T) { + srv := newTestServer(&stubEngine{ + recallFn: func(context.Context, engine.RecallOptions) ([]memory.Memory, error) { + return []memory.Memory{{ + Layer: memory.LayerDecision, + Domain: "api", + Path: "decisions/camelcase.md", + Body: "camelCase for API responses", + RevisitIf: "GraphQL adoption", + }}, nil + }, + }) + + res := srv.handleRecall(context.Background(), map[string]any{"domain": "api"}) + if res.IsError { + t.Fatalf("unexpected error result: %+v", res) + } + if !strings.Contains(res.Content[0].Text, "GraphQL adoption") { + t.Errorf("revisit_if missing from recall payload: %s", res.Content[0].Text) + } +} diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index 935423b..a65d070 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -45,6 +45,21 @@ var brainTools = []toolDefinition{ Type: "string", Description: "Comma-separated tags for the memory.", }, + "revisit_if": { + Type: "string", + Description: "The condition that would make this memory worth reconsidering (e.g. \"GraphQL adoption\"). " + + "Recorded and surfaced on recall; nothing evaluates it, so a human or tool decides when it has been met.", + }, + "supersedes": { + Type: "string", + Description: "Path of the memory this one replaces (e.g. \"decisions/old-choice.md\"), which is retired. " + + "A path that does not resolve is an error and nothing is stored.", + }, + "persona": { + Type: "string", + Description: "The persona this memory came from, as a lower-kebab-case slug (e.g. \"kent-beck\"). " + + "Matching a persona tracked by brain_track ranks this memory by that persona's acceptance rate in its domain.", + }, }, Required: []string{"content", "domain"}, }, @@ -175,7 +190,7 @@ var brainTools = []toolDefinition{ }, { Name: "brain_track", - Description: "Record a Council persona outcome (accepted/overridden) for a domain, or view current stats. Effectiveness is a rolling 90-day acceptance rate; the returned stats feed recall ranking when UseEffectiveness is set.", + Description: "Record a Council persona outcome (accepted/overridden) for a domain, or view current stats. Effectiveness is a rolling 90-day acceptance rate; the returned stats feed recall ranking on any domain-scoped recall.", InputSchema: toolSchema{ Type: "object", Properties: map[string]schemaProperty{