From 03ca97da3044d2487b421343ef06695ab996a8af Mon Sep 17 00:00:00 2001 From: Siwen Yu Date: Tue, 27 Oct 2026 17:32:03 +0800 Subject: [PATCH] fix(memory): require a base_url for the hindsight backend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{"memory": {"backend": "hindsight"}}` — no endpoint, just the backend name — built a client whose base_url fell back to a hard-coded service, so the config silently sent every prompt to a deployment the user never named. The decision record says a network backend is "reachable only by explicit configuration: an endpoint written in the user's own config file"; the code enforced the file and not the endpoint. `NewHTTPMemoryClient` now takes the bank and refuses an empty base URL, so `memoryStore` returns an error the composition root already logs and runs without memory. There is no default endpoint anywhere in the tree. The bank had the same problem in a smaller way: `hermes` was hard-coded while `memory.bank` existed in the config and was read by nobody. It now comes from the config and falls back to `default`, which is Hindsight's own default tenant name rather than one deployment's. Docs move with it: CODEBASE says the endpoint is required, the example config says a hindsight backend must name one, the decision record says the endpoint rule is enforced rather than stated, and the roadmap's B2 evidence keeps its baseline wording with a note that that default no longer exists. Verified: go test ./... -count=1, 16 packages ok; go vet and gofmt clean; the gate mutation-checked (the empty-base refusal disabled fails TestHTTPMemoryClientRequiresAnEndpoint, restored it passes). Refs #293 --- CODEBASE.md | 11 ++++--- README.md | 2 +- agent/memory.go | 40 ++++++++++++++-------- agent/memory_test.go | 73 ++++++++++++++++++++++++++++++----------- config.example.json | 2 +- docs/memory-decision.md | 3 +- docs/roadmap.md | 4 ++- main.go | 5 ++- memory_wiring_test.go | 27 +++++++++++++++ 9 files changed, 125 insertions(+), 42 deletions(-) diff --git a/CODEBASE.md b/CODEBASE.md index dd69849..f7babd7 100644 --- a/CODEBASE.md +++ b/CODEBASE.md @@ -1,6 +1,6 @@ # TinyCode — CODEBASE Map -> AI coding agent in pure Go. Single binary, Bubble Tea TUI, ReAct agent loop, 25 built-in tools + MCP, LSP diagnostics, session persistence. 1228 test functions + 10 fuzz targets, race-detector clean. +> AI coding agent in pure Go. Single binary, Bubble Tea TUI, ReAct agent loop, 25 built-in tools + MCP, LSP diagnostics, session persistence. 1230 test functions + 10 fuzz targets, race-detector clean. ## Quick Reference @@ -275,7 +275,8 @@ from every permission whitelist. `docs/memory-decision.md` is the policy behind What runs today, in order: 1. `main.go` resolves the `memory` section with `UserLayer.ResolveMemory`, which honours a network - backend or a `base_url` only from the user's own config file. + backend or a `base_url` only from the user's own config file, and `memoryStore` refuses a + network backend that names no `base_url` — there is no default endpoint to fall back to. 2. It builds the store with `memoryStore`: the local backend gets a per-project shard (`MemoryScopeFor`), the remote one gets the endpoint the user wrote — there is no default endpoint. 3. For the local backend it registers the directory with the tool layer (`tool.SetMemoryRoot`), so a @@ -303,7 +304,7 @@ What runs today, in order: `Forget` reports `ErrMemoryDeleteUnsupported` rather than returning nil: the service has no per-memory delete, and the adapter's recall/list decoders predate its current API (issue #293, S4). - Methods: `Remember(key, value string) error`, `Recall(query string, limit int) ([]types.Memory, error)`, `Forget(id string) error`, `List() ([]types.Memory, error)` -- Hindsight API: bank "hermes", 15s timeout, auth via `HINDSIGHT_API_KEY` or `OPENAI_API_KEY` +- Hindsight API: `base_url` is **required** (`memory.base_url`; there is no default endpoint and the client is not built without one), bank from `memory.bank` (default `default`), 15s timeout, auth via `HINDSIGHT_API_KEY` or `OPENAI_API_KEY` ### `truncate.go` — Output Truncation @@ -849,7 +850,7 @@ User Input (textarea / CLI arg) ## Testing -- **1228 test functions + 10 fuzz targets** in the main module — what `go test ./... -count=1` runs, since `tuiprobe/` is a nested module; each count below is listed with the command that produces it, and no count is carried in a copy that nothing checks +- **1230 test functions + 10 fuzz targets** in the main module — what `go test ./... -count=1` runs, since `tuiprobe/` is a nested module; each count below is listed with the command that produces it, and no count is carried in a copy that nothing checks - `make fuzz` (`FUZZTIME=30s`) explores every fuzz target; `go test` already runs their seed corpora, so CI exercises them on every push - `make test-browser` (`BROWSER_TEST=1`, `-run TestBrowserSmoke`) renders a JavaScript page through both browser paths against a loopback server and asserts the request carried the proxy's `Via` header, which proves the filtering proxy was used; `TestBrowserSmokeRefusesABlockedSubresource` then serves a page whose own authority is exempted from the loopback rule while the policy stays enforced, and asserts a subresource pointing at a *second, live* loopback service is refused (the script's `onerror` marker in the DOM) and that the service was never reached — a refusal is proven, not only that the proxy was used (issue #8). Both skip without a browser, so `make test` never launches one - `make test-tui-visual` (`TUI_SHOT=1`) is the only place the TUI is looked at rather than asserted on: it renders the committed frame scenarios to PNGs through headless Chromium, starts `bin/tinycode` on a real 80x24 PTY (asserting the stream carried SGR styling and that the binary quits on the documented double Ctrl+C), runs it once more on a size-less PTY, and replays the live stream into a screen buffer that is screenshotted as `tinycode-pty-welcome-80x24.png`. Both tests skip without the variable, so `make test` needs neither a browser, nor a binary, nor a terminal device; PNGs land in `TUI_SHOT_DIR` (default `/tmp`) @@ -895,7 +896,7 @@ Counts in this document are produced by these commands at the commit they descri ```bash find . -name '*_test.go' -not -path './tuiprobe/*' -print0 \ - | xargs -0 grep -h '^func Test' | wc -l # 1228 test functions (main module) + | xargs -0 grep -h '^func Test' | wc -l # 1230 test functions (main module) grep -rn '^func Fuzz' --include=*_test.go . | wc -l # 10 fuzz targets ls tui/testdata/golden/frames/*.txt | wc -l # 29 plain-text frames ls tui/testdata/golden/ansi/*.ansi | wc -l # 1 raw ANSI frame diff --git a/README.md b/README.md index 2c5a8f5..e8f1c9e 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,7 @@ Custom **CellGrid** frame-buffer renders markdown directly in the terminal — n - **GitHub Actions**: Two workflows — main.yml (build + lint + test on push/PR) and release.yml (cross-compile + GitHub Releases on tags v*) - **main.yml jobs**: `ci` (build, gofmt gate, vet, tests, `-race`, repeated run), `lsp` (real gopls), `browser` (real Chromium), `tui-visual` (frame PNGs + the built binary on a PTY), `cross` (linux/amd64, linux/arm64, darwin/arm64 build + vet) and `staticcheck` - **Makefile improvements**: test target preserves exit code with pass/fail message; releases target cross-compiles all platforms + .tar.gz archives -- **1228 test functions + 10 fuzz targets** in the main module — the one `go test ./... -count=1` runs, since `tuiprobe/` is a nested module — counted with `find . -name '*_test.go' -not -path './tuiprobe/*' -print0 | xargs -0 grep -h '^func Test' | wc -l` and `grep -rn '^func Fuzz' --include=*_test.go . | wc -l`; the command is part of the record, and `CODEBASE.md` → Testing lists every count with its measurement +- **1230 test functions + 10 fuzz targets** in the main module — the one `go test ./... -count=1` runs, since `tuiprobe/` is a nested module — counted with `find . -name '*_test.go' -not -path './tuiprobe/*' -print0 | xargs -0 grep -h '^func Test' | wc -l` and `grep -rn '^func Fuzz' --include=*_test.go . | wc -l`; the command is part of the record, and `CODEBASE.md` → Testing lists every count with its measurement - **Annotations**: the jobs carry one `ubuntu-latest` migration notice each, plus the `setup-chrome@v1` Node 20 warning on `browser` and `tui-visual`; the gate is no *new* annotations, not zero ### Running the checks locally diff --git a/agent/memory.go b/agent/memory.go index e6c6897..2cc4c1d 100644 --- a/agent/memory.go +++ b/agent/memory.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "os" + "strings" "time" "github.com/yusiwen/tinycode/types" @@ -19,24 +20,37 @@ import ( // and it is not (docs/memory-decision.md). var ErrMemoryDeleteUnsupported = errors.New("memory: this backend cannot delete a fact") -const hindsightBase = "https://memory.yusiwen.cn/dp-api" -const hindsightBank = "hermes" +// defaultMemoryBank is the bank a client uses when the configuration names +// none. Hindsight's own default tenant is called "default", so this is the +// service's convention rather than a guess about one deployment. +const defaultMemoryBank = "default" var hindsightHTTP = &http.Client{Timeout: 15 * time.Second} +// HTTPMemoryClient is the adapter for a Hindsight deployment. +// +// There is no default endpoint and no default bank: an endpoint is where the +// prompt goes, and a client that invents one sends someone's text to a service +// they never configured. The bank is a name inside that deployment, so it falls +// back to the service's own default. type HTTPMemoryClient struct { BaseURL string + Bank string } -func NewHTTPMemoryClient(baseURL string) *HTTPMemoryClient { - return &HTTPMemoryClient{BaseURL: baseURL} -} - -func (c *HTTPMemoryClient) base() string { - if c.BaseURL != "" { - return c.BaseURL +// NewHTTPMemoryClient builds the adapter for one endpoint. A missing base URL is +// an error rather than a fallback, because a fallback is the bug this signature +// exists to prevent. +func NewHTTPMemoryClient(baseURL, bank string) (*HTTPMemoryClient, error) { + base := strings.TrimSpace(baseURL) + if base == "" { + return nil, errors.New("memory: the hindsight backend needs a base_url") + } + bank = strings.TrimSpace(bank) + if bank == "" { + bank = defaultMemoryBank } - return hindsightBase + return &HTTPMemoryClient{BaseURL: base, Bank: bank}, nil } func (c *HTTPMemoryClient) auth() string { @@ -79,13 +93,13 @@ func (c *HTTPMemoryClient) do(ctx context.Context, method, url string, body, dst } func (c *HTTPMemoryClient) Remember(key, value string) error { - u := c.base() + "/v1/default/banks/" + hindsightBank + "/memories" + u := c.BaseURL + "/v1/default/banks/" + c.Bank + "/memories" body := map[string]any{"items": []map[string]string{{"content": value, "context": key}}} return c.do(context.Background(), http.MethodPost, u, body, nil) } func (c *HTTPMemoryClient) Recall(ctx context.Context, query string, limit int) ([]types.Memory, error) { - u := c.base() + "/v1/default/banks/" + hindsightBank + "/memories/recall" + u := c.BaseURL + "/v1/default/banks/" + c.Bank + "/memories/recall" body := map[string]any{"query": query, "budget": "mid"} var raw struct { Results []struct { @@ -115,7 +129,7 @@ func (c *HTTPMemoryClient) Forget(id string) error { } func (c *HTTPMemoryClient) List() ([]types.Memory, error) { - u := c.base() + "/v1/default/banks/" + hindsightBank + "/memories/list" + u := c.BaseURL + "/v1/default/banks/" + c.Bank + "/memories/list" var raw struct { MemoryUnits []struct { Content string `json:"content"` diff --git a/agent/memory_test.go b/agent/memory_test.go index 480fd75..1e2f439 100644 --- a/agent/memory_test.go +++ b/agent/memory_test.go @@ -11,6 +11,47 @@ import ( "testing" ) +// newMemoryClient builds the adapter for a stub server, with a bank the test +// chooses so nothing here depends on a deployment's name. +func newMemoryClient(t *testing.T, baseURL string) *HTTPMemoryClient { + t.Helper() + c, err := NewHTTPMemoryClient(baseURL, "testbank") + if err != nil { + t.Fatalf("NewHTTPMemoryClient(%q): %v", baseURL, err) + } + return c +} + +// TestHTTPMemoryClientRequiresAnEndpoint pins the signature that replaced a +// hard-coded default: a network backend that was not told where to send the +// prompt is not built at all, and the bank falls back to the service's own +// default rather than to one deployment's name. +func TestHTTPMemoryClientRequiresAnEndpoint(t *testing.T) { + if _, err := NewHTTPMemoryClient("", "testbank"); err == nil { + t.Fatal("an empty base URL was accepted") + } else if !strings.Contains(err.Error(), "base_url") { + t.Fatalf("the error does not name the missing key: %v", err) + } + if _, err := NewHTTPMemoryClient(" ", ""); err == nil { + t.Fatal("a blank base URL was accepted") + } + + c, err := NewHTTPMemoryClient("http://127.0.0.1:9070/", "") + if err != nil { + t.Fatalf("a configured endpoint: %v", err) + } + if c.BaseURL != "http://127.0.0.1:9070/" { + t.Fatalf("base = %q", c.BaseURL) + } + if c.Bank != defaultMemoryBank { + t.Fatalf("bank = %q, want the service default %q", c.Bank, defaultMemoryBank) + } + named, err := NewHTTPMemoryClient("http://127.0.0.1:9070", "work") + if err != nil || named.Bank != "work" { + t.Fatalf("named bank = %+v, %v", named, err) + } +} + // TestHTTPMemoryClient covers the Hindsight-backed memory store end to end // against a stub server: request shape, bearer token, response mapping and the // limit applied to recalls. @@ -29,11 +70,11 @@ func TestHTTPMemoryClient(t *testing.T) { _ = json.Unmarshal(lastBody, &gotBody) switch r.URL.Path { - case "/v1/default/banks/hermes/memories": + case "/v1/default/banks/testbank/memories": w.WriteHeader(http.StatusOK) - case "/v1/default/banks/hermes/memories/recall": + case "/v1/default/banks/testbank/memories/recall": io.WriteString(w, `{"results":[{"content":"v1","context":"k1"},{"content":"v2","context":"k2"}]}`) - case "/v1/default/banks/hermes/memories/list": + case "/v1/default/banks/testbank/memories/list": io.WriteString(w, `{"memory_units":[{"content":"lv","context":"lk"}]}`) default: http.NotFound(w, r) @@ -50,11 +91,11 @@ func TestHTTPMemoryClient(t *testing.T) { t.Setenv("HINDSIGHT_API_KEY", "secret") t.Setenv("OPENAI_API_KEY", "") - c := NewHTTPMemoryClient(srv.URL) + c := newMemoryClient(t, srv.URL) if err := c.Remember("k", "v"); err != nil { t.Fatalf("Remember: %v", err) } - if gotMethod != http.MethodPost || gotPath != "/v1/default/banks/hermes/memories" { + if gotMethod != http.MethodPost || gotPath != "/v1/default/banks/testbank/memories" { t.Fatalf("Remember used %s %s", gotMethod, gotPath) } if gotAuth != "Bearer secret" { @@ -97,17 +138,11 @@ func TestHTTPMemoryClient(t *testing.T) { t.Fatalf("Forget = %v, want ErrMemoryDeleteUnsupported", err) } - // An empty base URL falls back to the built-in Hindsight endpoint, and the - // token falls back to OPENAI_API_KEY when no Hindsight key is configured. - if got := NewHTTPMemoryClient("").base(); got != hindsightBase { - t.Fatalf("default base = %q, want %q", got, hindsightBase) - } - if got := NewHTTPMemoryClient(srv.URL).base(); got != srv.URL { - t.Fatalf("configured base = %q, want %q", got, srv.URL) - } + // The token still falls back to OPENAI_API_KEY when no Hindsight key is + // configured (a separate decision, tracked as S4 of issue #293). t.Setenv("HINDSIGHT_API_KEY", "") t.Setenv("OPENAI_API_KEY", "openai-key") - if err := NewHTTPMemoryClient(srv.URL).Remember("k", "v"); err != nil { + if err := newMemoryClient(t, srv.URL).Remember("k", "v"); err != nil { t.Fatalf("Remember with the OpenAI fallback: %v", err) } if gotAuth != "Bearer openai-key" { @@ -142,25 +177,25 @@ func TestHTTPMemoryClientErrors(t *testing.T) { {"recall non-2xx", func(c *HTTPMemoryClient) error { _, err := c.Recall(context.Background(), "q", 1); return err }, "500"}, {"list non-2xx", func(c *HTTPMemoryClient) error { _, err := c.List(); return err }, "500"}, {"recall bad json", func(c *HTTPMemoryClient) error { - _, err := NewHTTPMemoryClient(broken.URL).Recall(context.Background(), "q", 1) + _, err := newMemoryClient(t, broken.URL).Recall(context.Background(), "q", 1) return err }, "invalid"}, {"list bad json", func(c *HTTPMemoryClient) error { - _, err := NewHTTPMemoryClient(broken.URL).List() + _, err := newMemoryClient(t, broken.URL).List() return err }, "invalid"}, } for _, tc := range tests { - if err := tc.call(NewHTTPMemoryClient(failing.URL)); err == nil || !strings.Contains(err.Error(), tc.want) { + if err := tc.call(newMemoryClient(t, failing.URL)); err == nil || !strings.Contains(err.Error(), tc.want) { t.Errorf("%s: error = %v, want it to mention %q", tc.name, err, tc.want) } } - if err := NewHTTPMemoryClient(deadURL).Remember("k", "v"); err == nil { + if err := newMemoryClient(t, deadURL).Remember("k", "v"); err == nil { t.Error("a closed server must surface a transport error") } - c := NewHTTPMemoryClient(failing.URL) + c := newMemoryClient(t, failing.URL) if err := c.do(context.Background(), http.MethodPost, failing.URL, make(chan int), nil); err == nil || !strings.Contains(err.Error(), "marshal") { t.Errorf("unmarshalable body: error = %v, want a marshal error", err) } diff --git a/config.example.json b/config.example.json index b332dc2..791d5b6 100644 --- a/config.example.json +++ b/config.example.json @@ -181,7 +181,7 @@ }, "memory": { - "$comment": "Long-term memory. ON by default, local, and it stores nothing unless you ask: /remember is the only writer, and recall reads one file per project under dir (default ~/.tinycode/memory, deliberately outside the project root and refused to tools, because a fact the agent can rewrite is not one). backend is 'local' or 'hindsight'; a network backend, or any base_url, sends each run's prompt to that service, so it is honoured only when it comes from your own ~/.tinycode/config.json — a project-local .tinycode/config.json that names one is ignored and the ignore is logged. max_entries and max_bytes bound the store (oldest facts dropped first); recall_limit is how many facts one run may inject. The store is lexical today: embeddings are roadmap B3, so an absent keyword is an absent fact. See docs/memory-decision.md.", + "$comment": "Long-term memory. ON by default, local, and it stores nothing unless you ask: /remember is the only writer, and recall reads one file per project under dir (default ~/.tinycode/memory, deliberately outside the project root and refused to tools, because a fact the agent can rewrite is not one). backend is 'local' or 'hindsight'. A network backend sends each run's prompt to that service, so it is honoured only when it comes from your own ~/.tinycode/config.json — a project-local .tinycode/config.json that names one is ignored and the ignore is logged. 'hindsight' requires base_url (there is no default endpoint) and takes its bank from 'bank' (default 'default'). max_entries and max_bytes bound the store (oldest facts dropped first); recall_limit is how many facts one run may inject. The store is lexical today: embeddings are roadmap B3, so an absent keyword is an absent fact. See docs/memory-decision.md.", "enabled": true, "backend": "local", "max_entries": 2000, diff --git a/docs/memory-decision.md b/docs/memory-decision.md index 54fe77c..0213dcc 100644 --- a/docs/memory-decision.md +++ b/docs/memory-decision.md @@ -47,7 +47,8 @@ on every run, and the difference is not a setting. - **`/memory`** lists what is stored and **`/memory forget `** removes it, by the id the listing shows. A store without a way to read and delete it is not one a person can consent to. - **A network backend (Hindsight)** is reachable only by explicit configuration: an endpoint written in - the user's own config file. A project-local `.tinycode/config.json` may not enable egress — the same + the user's own config file, and nothing else — the client is not built without one, so a config that + names the backend and no endpoint is an error at startup rather than a default. A project-local `.tinycode/config.json` may not enable egress — the same rule `capture: full` follows in `config/config.go:928`. The adapter as it stands also predates the current Hindsight API: upstream recall results carry `text` (the client decodes `content`) and the listing returns `items` (the client decodes `memory_units`), while `Forget` is a documented no-op at diff --git a/docs/roadmap.md b/docs/roadmap.md index e1d957f..9a1bf40 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -333,7 +333,9 @@ current tree. D7 is the exception: it was recorded later, so its references are privacy-sensitive use is impossible; there is also no way to see, review or approve what is remembered. - **Evidence**: `MemoryStore`'s only implementation is `HTTPMemoryClient` (`agent/memory.go:74-120`), - defaulting to `https://memory.yusiwen.cn/dp-api`, bank `hermes` (`agent/memory.go:15-16`); + defaulting to `https://memory.yusiwen.cn/dp-api`, bank `hermes` (`agent/memory.go:15-16`) — that + default no longer exists: `base_url` is required and the bank comes from config, see the + **Picked up** line above; `Forget` is a no-op that returns nil (`agent/memory.go:102-105`), so deletion is not merely unexposed but unimplemented; `rg -il 'sqlite|local.?memory|memory.?backend'` → exit 1. - **Decision (2026-10-25)**: the local store is the only writer in v1, and its writer is the user diff --git a/main.go b/main.go index 61fb7f4..1cdd0c1 100644 --- a/main.go +++ b/main.go @@ -1219,7 +1219,10 @@ func providerLabel(pc config.ProviderRecordConfig, index int) string { // reads them, which is the same rule the rest of memory follows. func memoryStore(spec config.MemorySpec, projectRoot string) (types.MemoryStore, error) { if spec.Backend == config.MemoryBackendHindsight { - return agent.NewHTTPMemoryClient(spec.BaseURL), nil + // No default endpoint: a network backend that was not told where to send + // the prompt is not built at all (the caller logs the error and runs + // without memory). + return agent.NewHTTPMemoryClient(spec.BaseURL, spec.Bank) } local, err := agent.NewLocalMemory(spec.Dir, agent.MemoryScopeFor(projectRoot), spec.MaxEntries, spec.MaxBytes) if err != nil { diff --git a/memory_wiring_test.go b/memory_wiring_test.go index e6b587b..434d932 100644 --- a/memory_wiring_test.go +++ b/memory_wiring_test.go @@ -3,6 +3,7 @@ package main import ( "os" "path/filepath" + "strings" "testing" "github.com/yusiwen/tinycode/agent" @@ -49,6 +50,32 @@ func TestMemoryStoreBuildsTheConfiguredBackend(t *testing.T) { if client.BaseURL != remote.BaseURL { t.Fatalf("client base = %q, want %q", client.BaseURL, remote.BaseURL) } + if client.Bank != "default" { + t.Fatalf("bank = %q, want the service default", client.Bank) + } + + remote.Bank = "work" + if store, err = memoryStore(remote, ""); err != nil { + t.Fatalf("named bank: %v", err) + } + if got := store.(*agent.HTTPMemoryClient).Bank; got != "work" { + t.Fatalf("bank = %q, want work", got) + } +} + +// TestMemoryStoreRefusesHindsightWithoutAnEndpoint pins the fix for the silently +// defaulted endpoint: a network backend that names no base_url is an error, so +// the caller logs it and runs without memory rather than sending the prompt to +// a service nobody configured. +func TestMemoryStoreRefusesHindsightWithoutAnEndpoint(t *testing.T) { + spec := config.MemorySpec{Enabled: true, Backend: config.MemoryBackendHindsight} + store, err := memoryStore(spec, "") + if err == nil { + t.Fatalf("a hindsight spec with no base_url built %T", store) + } + if !strings.Contains(err.Error(), "base_url") { + t.Fatalf("the error does not name the missing key: %v", err) + } } // TestMemoryStoreReportsAnUnusableDirectory pins that a store path that cannot