From 9d9ad5f8eb98615c7dd981c38ece2562cf4f8886 Mon Sep 17 00:00:00 2001 From: "voxist.executor" Date: Thu, 6 Aug 2026 13:35:59 +0000 Subject: [PATCH 1/4] fix(session): carry the no_history storage policy through the create front door MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vp-ia76 — phase 1 of vp-9u1. The stop-the-bleed change, REBASED onto fork/main: the first attempt was built on a STALE origin/main ref (242 commits divergent, from a remote that no longer exists) — which is what made the pre-push gate's path tests fail, since their fix already existed upstream. Verified by applying this same patch to fork/main and watching those tests pass unchanged. CreateSessionInfo is the SINGLE front door for session-bead creation, and it called s.store.Create(), which carries no storage class. gascity's own policy (beadPolicySession -> beadStorageNoHistory) was therefore dropped, and every session bead landed in the Dolt-COMMITTED issues table instead of the dolt_ignore'd wisps table, one DOLT_COMMIT each. Measured: 262 sessions/24h. That commit volume grows hq, drives compaction, and rebuilds the push backlog faster than the 15s listener window can ship it (the ratchet measured 2026-08-05/06). no_history, NOT ephemeral: ephemeral sets ephemeral=1, which bead_policy_store declares incompatible for sessions and matchesTier silently DROPS. The 204 session rows already in hq.wisps are no_history=1, ephemeral=0 — matched here. THE FALLBACK IS LOUD. CachingStore.CreateWithStorage silently degrades to plain Create when the backing store lacks StorageCreateStore — and NativeDoltStore lacks it, so this fix ALONE is inert on a native-store chain: the warning is what makes that visible instead of silent. The COMPANION fix that makes the native store honor the class exists as branch vp-ia76-session-nohistory (c2382c780, built 2026-07-31, never pushed); both are needed for the commit volume to actually drop. This commit deliberately does not absorb it — it is another author's work and should land as itself. Verified red-first on this base: restoring the plain Create -> 6 failures. internal/session ok, internal/beads ok, go vet clean. --- internal/session/create.go | 66 ++++++++++- internal/session/create_storage_test.go | 144 ++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 internal/session/create_storage_test.go diff --git a/internal/session/create.go b/internal/session/create.go index 6b2ca373ba..e73a3fd16a 100644 --- a/internal/session/create.go +++ b/internal/session/create.go @@ -1,6 +1,12 @@ package session -import "github.com/gastownhall/gascity/internal/beads" +import ( + "fmt" + "os" + "sync" + + "github.com/gastownhall/gascity/internal/beads" +) // CreateSpec captures the typed vocabulary for creating a session bead through // the front door. It is the byte-identical replacement for the inline @@ -57,19 +63,73 @@ type CreateSpec struct { // pinned across every backend by the beadstest conformance case // CreateEchoMatchesGetOnMetadata, not just by the memstore-backed oracle here. func (s *Store) CreateSessionInfo(spec CreateSpec) (Info, error) { - created, err := s.store.Create(beads.Bead{ + bead := beads.Bead{ ID: spec.ID, Title: spec.Title, Type: BeadType, Labels: []string{LabelSession, "agent:" + spec.AgentName}, Metadata: spec.Metadata, - }) + } + created, err := s.createSessionBead(bead) if err != nil { return Info{}, err } return infoFromPersistedBead(created), nil } +// createSessionBead persists a session bead under the SESSION STORAGE POLICY. +// +// WHY THIS EXISTS (vp-ia76, phase 1 of vp-9u1). This front door used to call +// s.store.Create() directly, which carries no storage class. gascity's own policy +// (cmd/gc/bead_policy_store.go: beadPolicySession -> beadStorageNoHistory) was +// therefore dropped on the floor, and every session bead landed in the Dolt-COMMITTED +// issues table instead of the dolt_ignore'd wisps table — one DOLT_COMMIT each. +// Measured 2026-07-30: 262 sessions/24h through this door into issues, against 110 +// into wisps through the policy-honoring controller path. That commit volume is what +// grows the hq store, drives compaction, and rebuilds the push backlog faster than the +// 15s listener window can ship it. +// +// no_history, NOT ephemeral. They are not interchangeable: +// - no_history: row in wisps, ephemeral=0, no DOLT_COMMIT, NOT GC/TTL-eligible, +// reads keep working. The 204 session rows already in hq.wisps have exactly this +// shape (no_history=1, ephemeral=0) — this matches them. +// - ephemeral: also GC/TTL-eligible, sets ephemeral=1, which gascity's own policy +// declares incompatible for sessions (bead_policy_store.go) and which +// matchesTier (internal/beads/query.go) silently DROPS from results. +// +// THE FALLBACK IS LOUD ON PURPOSE. CachingStore.CreateWithStorage silently degrades to +// a plain Create when its backing store does not implement StorageCreateStore — and +// NativeDoltStore does not implement it. So a chain assembled the wrong way would take +// this fix, report success, and keep writing to issues exactly as before: the fix would +// be INERT AND INVISIBLE, which is the failure mode this change exists to end +// (ADR-0043: an unknown must propagate, not be coerced into the quiet answer). When the +// class cannot be honored we say so rather than pretend, once per process so a hot path +// cannot spam the ops tail. +func (s *Store) createSessionBead(b beads.Bead) (beads.Bead, error) { + storageStore, ok := s.store.Store.(beads.StorageCreateStore) + if !ok { + warnSessionStorageUnsupported(s.store.Store) + return s.store.Create(b) + } + return storageStore.CreateWithStorage(b, beads.StorageNoHistory) +} + +var sessionStorageWarnOnce sync.Once + +// warnSessionStorageUnsupported reports, once per process, that the session storage +// policy could not be applied. Silence here would mean the caller believes sessions are +// staying out of the committed table while they are not. +func warnSessionStorageUnsupported(store any) { + sessionStorageWarnOnce.Do(func() { + fmt.Fprintf(os.Stderr, + "gc: session storage policy NOT applied: %T does not implement "+ + "beads.StorageCreateStore, so session beads are being written to the "+ + "committed issues table (one DOLT_COMMIT each) instead of wisps. "+ + "This is vp-ia76 / vp-9u1 and it silently inflates the store.\n", + store) + }) +} + // CreateSession creates a session bead from spec and returns its id. It is the // id-only sibling of CreateSessionInfo (the single front door for session-bead // creation) and delegates to it, so both emit the byte-identical Create; callers diff --git a/internal/session/create_storage_test.go b/internal/session/create_storage_test.go new file mode 100644 index 0000000000..1ff1dd5141 --- /dev/null +++ b/internal/session/create_storage_test.go @@ -0,0 +1,144 @@ +package session + +import ( + "bytes" + "io" + "os" + "strings" + "sync" + "testing" + + "github.com/gastownhall/gascity/internal/beads" +) + +// recordingStorageStore implements both Create and CreateWithStorage so a test can +// tell WHICH door a create went through. Deliberately not a mock of the function +// under test: it is a store, and CreateSessionInfo drives it for real. +type recordingStorageStore struct { + beads.Store + plainCreates int + storageCreates int + lastStorage beads.StorageClass +} + +func (r *recordingStorageStore) Create(b beads.Bead) (beads.Bead, error) { + r.plainCreates++ + return b, nil +} + +func (r *recordingStorageStore) CreateWithStorage(b beads.Bead, storage beads.StorageClass) (beads.Bead, error) { + r.storageCreates++ + r.lastStorage = storage + return b, nil +} + +// plainOnlyStore implements Create but NOT CreateWithStorage — the shape of +// NativeDoltStore, which is why the silent-fallback hazard is real. +type plainOnlyStore struct { + beads.Store + plainCreates int +} + +func (p *plainOnlyStore) Create(b beads.Bead) (beads.Bead, error) { + p.plainCreates++ + return b, nil +} + +func newSpec() CreateSpec { + return CreateSpec{ID: "vc-wisp-test1", Title: "t", AgentName: "a"} +} + +// A session bead MUST be created under the no_history storage class. Before vp-ia76 +// this front door called Create() directly, so gascity's own session policy was +// dropped and every session landed in the committed issues table with its own +// DOLT_COMMIT — 262/24h, measured. +func TestCreateSessionInfoAppliesNoHistoryStorage(t *testing.T) { + rec := &recordingStorageStore{} + s := NewStore(beads.SessionStore{Store: rec}) + + if _, err := s.CreateSessionInfo(newSpec()); err != nil { + t.Fatalf("CreateSessionInfo: %v", err) + } + if rec.storageCreates != 1 { + t.Errorf("CreateWithStorage calls = %d, want 1 "+ + "(the session storage policy was dropped; beads land in the committed "+ + "issues table with a DOLT_COMMIT each)", rec.storageCreates) + } + if rec.plainCreates != 0 { + t.Errorf("plain Create calls = %d, want 0 (policy bypassed)", rec.plainCreates) + } + if rec.lastStorage != beads.StorageNoHistory { + t.Errorf("storage class = %q, want %q", rec.lastStorage, beads.StorageNoHistory) + } +} + +// no_history and ephemeral are NOT interchangeable. ephemeral sets ephemeral=1, which +// gascity's own policy declares incompatible for sessions and which matchesTier +// silently DROPS from query results — so using it would make sessions vanish from +// reads while looking like a successful fix. +func TestSessionStorageIsNoHistoryAndNotEphemeral(t *testing.T) { + rec := &recordingStorageStore{} + s := NewStore(beads.SessionStore{Store: rec}) + + if _, err := s.CreateSessionInfo(newSpec()); err != nil { + t.Fatalf("CreateSessionInfo: %v", err) + } + if rec.lastStorage == beads.StorageEphemeral { + t.Fatal("session beads must NOT be created ephemeral: ephemeral=1 is declared " + + "incompatible for sessions by bead_policy_store.go and matchesTier drops " + + "such rows from query results") + } + if rec.lastStorage != beads.StorageNoHistory { + t.Fatalf("storage class = %q, want %q", rec.lastStorage, beads.StorageNoHistory) + } +} + +// THE FIX MUST NOT BE ABLE TO SHIP INERT. CachingStore.CreateWithStorage silently +// degrades to Create when its backing store lacks StorageCreateStore, and +// NativeDoltStore lacks it. A chain assembled that way would take this fix, report +// success, and keep writing to issues. The create must still succeed — observability +// must never break the caller — but it must be REPORTED, not silent. +func TestUnsupportedStorageStillCreatesButIsReported(t *testing.T) { + plain := &plainOnlyStore{} + s := NewStore(beads.SessionStore{Store: plain}) + + // Capture stderr for real. Asserting only that the create SUCCEEDED would make + // this test named "...IsReported" while proving nothing about reporting — a guard + // that cannot fail, which is the exact pattern this codebase keeps producing. + old := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + os.Stderr = w + sessionStorageWarnOnce = sync.Once{} + + _, createErr := s.CreateSessionInfo(newSpec()) + + if err := w.Close(); err != nil { + t.Fatalf("close stderr pipe: %v", err) + } + os.Stderr = old + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("read captured stderr: %v", err) + } + + if createErr != nil { + t.Fatalf("CreateSessionInfo must not fail when storage is unsupported: %v", createErr) + } + if plain.plainCreates != 1 { + t.Errorf("plain Create calls = %d, want 1 (the bead must still be persisted)", + plain.plainCreates) + } + if !strings.Contains(buf.String(), "session storage policy NOT applied") { + t.Errorf("no warning on stderr when the storage class could not be applied; "+ + "the fix would ship INERT and INVISIBLE — sessions keep landing in the "+ + "committed issues table while the change reports success. got: %q", + buf.String()) + } + if !strings.Contains(buf.String(), "plainOnlyStore") { + t.Errorf("warning does not name the offending store type, so an operator "+ + "cannot tell which chain dropped the policy. got: %q", buf.String()) + } +} From b1106f9fe40e23420fa844de1f50560a1e63a8da Mon Sep 17 00:00:00 2001 From: "voxist.executor" Date: Fri, 31 Jul 2026 15:48:06 +0000 Subject: [PATCH 2/4] fix(beads): carry policy storage class to the native store (vp-ia76) A session bead created through the front door is supposed to land in the dolt_ignore'd wisps table, where it costs no DOLT_COMMIT on create or on any later update. On the live hq city it lands in the committed issues table instead: 727 of 730 session beads created in 24h, and 21,911 of 22,427 rows in hq.issues (97.7%) are session beads. The policy layer was not the problem. beadPolicyStore.Create already resolves a session bead to the no_history class and calls CreateWithStorage. The class was discarded one layer lower: caching_store_writes.go storageBacking, ok := c.backing.(StorageCreateStore) if !ok { return c.Create(b) } *NativeDoltStore -- the store the live city runs on, as its "gc: update bead " commit messages show -- implements Create but not CreateWithStorage, so the assertion failed and the class was dropped with no error and no warning. *BdStore does implement it, which is why the few scopes still on the bd subprocess store produce correct wisp rows while everything else does not. That is ADR-0043 Cause 1: an unsupported capability coerced into the quiet default, sharing a code path with the definite negative. - NativeDoltStore gains CreateWithStorage. The tier rides on the issue's own Ephemeral/NoHistory fields, which is what the upstream beads layer routes on and what makes it skip DOLT_COMMIT. - The caching store's incapable-backing fallback now stamps the class onto the bead instead of discarding it, so the policy still lands on any backend that lacks the method. - beadWithStorageClass is the shared field-carrying form of a storage class. Guards were watched red before the fix, against the real call rather than a mock (ADR-0043 Rule 4). The capable-backend control stayed green throughout, so the pair localizes the defect instead of only reporting it. native_dolt_storage_class_live_test.go drives a real NativeDoltStore against a real Dolt sql-server and measures commits: plain create -> issues, 1 commit; no-history create -> wisps, 0 commits; 10 updates -> 10 commits vs 0. --- cmd/gc/session_storage_policy_wiring_test.go | 147 +++++++++++++++++ internal/beads/bdstore.go | 19 +++ internal/beads/caching_store_writes.go | 26 ++- .../native_dolt_storage_class_live_test.go | 152 ++++++++++++++++++ internal/beads/native_dolt_store.go | 17 ++ 5 files changed, 355 insertions(+), 6 deletions(-) create mode 100644 cmd/gc/session_storage_policy_wiring_test.go create mode 100644 internal/beads/native_dolt_storage_class_live_test.go diff --git a/cmd/gc/session_storage_policy_wiring_test.go b/cmd/gc/session_storage_policy_wiring_test.go new file mode 100644 index 0000000000..0d7c97ef8e --- /dev/null +++ b/cmd/gc/session_storage_policy_wiring_test.go @@ -0,0 +1,147 @@ +package main + +import ( + "context" + "testing" + + "github.com/gastownhall/gascity/internal/beads" + "github.com/gastownhall/gascity/internal/config" + "github.com/gastownhall/gascity/internal/session" +) + +// capableBackingStore is a backend that DOES implement the optional +// StorageCreateStore capability, like *beads.BdStore. It stands in for the +// deployments where session beads already route to the wisps table. +type capableBackingStore struct { + beads.Store + created []beads.Bead +} + +func (s *capableBackingStore) Create(b beads.Bead) (beads.Bead, error) { + s.created = append(s.created, b) + return s.Store.Create(b) +} + +func (s *capableBackingStore) CreateWithStorage(b beads.Bead, storage beads.StorageClass) (beads.Bead, error) { + b.Ephemeral = storage == beads.StorageEphemeral + b.NoHistory = storage == beads.StorageNoHistory + s.created = append(s.created, b) + return s.Store.Create(b) +} + +// incapableBackingStore is a backend that does NOT implement +// StorageCreateStore. This is the shape *beads.NativeDoltStore has today: it +// implements Create (and ApplyGraphPlanWithStorage) but no single-bead +// CreateWithStorage. It is the store the live hq city runs on — proven by the +// "gc: update bead " commit messages in hq.dolt_log, which are emitted only +// by NativeDoltStore.Update (internal/beads/native_dolt_store.go:1004). +// +// It records the bead exactly as it arrives, so the test can see whether the +// no-history storage class survived the trip. Routing to the wisps table is +// decided downstream by the beads library on issue.NoHistory +// (beads internal/storage/dolt/issues.go:26), so a bead that arrives with +// NoHistory=false lands in the committed issues table and costs a DOLT_COMMIT. +type incapableBackingStore struct { + beads.Store + created []beads.Bead +} + +func (s *incapableBackingStore) Create(b beads.Bead) (beads.Bead, error) { + s.created = append(s.created, b) + return s.Store.Create(b) +} + +func (s *incapableBackingStore) lastCreated(t *testing.T) beads.Bead { + t.Helper() + if len(s.created) == 0 { + t.Fatal("no create reached the backing store") + } + return s.created[len(s.created)-1] +} + +func sessionCreateSpec() session.CreateSpec { + return session.CreateSpec{ + Title: "voxist.planner", + AgentName: "voxist.planner", + Metadata: map[string]string{"state": "start_pending"}, + } +} + +// controllerCityStore composes a backing store exactly as the controller does +// (cmd/gc/api_state.go): openStoreResultAtForCityWithMode policy-wraps the +// opened store, then wrapWithCachingStore unwraps it, inserts the CachingStore +// and re-wraps the policy layer on the outside. +func controllerCityStore(t *testing.T, backing beads.Store) beads.Store { + t.Helper() + cfg := &config.City{} + cityStore := wrapWithCachingStore(context.Background(), wrapStoreWithBeadPolicies(backing, cfg), nil, false) + return beads.SessionStore{Store: resolveSessionStore(cityStore, cfg, t.TempDir(), nil)}.Store +} + +// TestSessionCreateRoutesToNoHistoryOnCapableBackend pins the behavior that +// already works, so the pair of tests localizes the defect rather than just +// reporting one. On a backend that implements StorageCreateStore the session +// policy survives end to end. +func TestSessionCreateRoutesToNoHistoryOnCapableBackend(t *testing.T) { + backing := &capableBackingStore{Store: beads.NewMemStore()} + + if _, err := sessionFrontDoor(controllerCityStore(t, backing)).CreateSessionInfo(sessionCreateSpec()); err != nil { + t.Fatalf("CreateSessionInfo: %v", err) + } + if len(backing.created) == 0 { + t.Fatal("no create reached the backing store") + } + if got := backing.created[len(backing.created)-1]; !got.NoHistory { + t.Fatalf("session create on a capable backend: NoHistory = false, want true") + } +} + +// TestSessionCreateRoutesToNoHistoryOnIncapableBackend is the vp-ia76 guard. +// +// A backend that cannot honor a storage class must not cause the class to be +// discarded. CachingStore.CreateWithStorage currently falls back to plain +// Create when the backing store is not a StorageCreateStore +// (internal/beads/caching_store_writes.go:20-22), dropping the policy silently +// — no error, no warning, no signal of any kind. That is ADR-0043 Cause 1: +// an unsupported capability coerced into the quiet default. +// +// The consequence is measurable on the live fleet, not theoretical. Measured on +// hq 2026-07-31: 727 of 730 session beads created in 24h landed in the +// committed issues table; 21,911 of 22,427 rows in hq.issues (97.7%) are +// session beads; and over a 6h window 2,885 of 3,363 Dolt commits were +// "bd: update ". Scratch-store measurement (bd 1.1.0, server mode, +// the fleet's configuration): a session bead in issues costs one Dolt commit on +// create and one on every update; the same bead in wisps costs zero for both. +// +// Reverting the fix must turn this red. It asserts the bead the backend +// actually receives — the value the beads library routes on — not that some +// wrapper was called. +func TestSessionCreateRoutesToNoHistoryOnIncapableBackend(t *testing.T) { + backing := &incapableBackingStore{Store: beads.NewMemStore()} + + if _, err := sessionFrontDoor(controllerCityStore(t, backing)).CreateSessionInfo(sessionCreateSpec()); err != nil { + t.Fatalf("CreateSessionInfo: %v", err) + } + got := backing.lastCreated(t) + if !got.NoHistory { + t.Fatal("session create reached a storage-class-incapable backend with NoHistory = false, want true: " + + "the no-history policy was dropped, so the bead lands in the committed issues table " + + "and costs a DOLT_COMMIT on create and on every subsequent update") + } + if got.Ephemeral { + t.Fatal("session create reached the backend with Ephemeral = true, want false: " + + "ephemeral beads are GC/TTL-eligible and are declared incompatible for sessions") + } +} + +// TestNativeDoltStoreDeclaresStorageCreateCapability pins the capability +// itself. The live city store is a *beads.NativeDoltStore; without this +// assertion the drop above is reachable again the moment the fallback is +// touched. +func TestNativeDoltStoreDeclaresStorageCreateCapability(t *testing.T) { + var store any = (*beads.NativeDoltStore)(nil) + if _, ok := store.(beads.StorageCreateStore); !ok { + t.Fatal("*beads.NativeDoltStore does not implement beads.StorageCreateStore: " + + "policy-selected storage classes are silently discarded on the store the live city runs on") + } +} diff --git a/internal/beads/bdstore.go b/internal/beads/bdstore.go index e221b711ce..1678737a92 100644 --- a/internal/beads/bdstore.go +++ b/internal/beads/bdstore.go @@ -1046,6 +1046,25 @@ func effectiveStorageFlags(b Bead, storage StorageClass) (ephemeral bool, noHist } } +// beadWithStorageClass returns b with its Ephemeral/NoHistory fields set to the +// storage class. It is the field-carrying form of a storage class, for the +// create paths that cannot pass the class out of band as a flag: every backend +// routes a bead to the wisps table on these fields, so stamping them is what +// makes a policy-selected class survive a store that does not implement the +// optional StorageCreateStore capability. StorageDefault leaves b untouched. +func beadWithStorageClass(b Bead, storage StorageClass) (Bead, error) { + ephemeral, noHistory, err := effectiveStorageFlags(b, storage) + if err != nil { + return Bead{}, err + } + if ephemeral && noHistory { + return Bead{}, fmt.Errorf("ephemeral and no-history storage are mutually exclusive") + } + b.Ephemeral = ephemeral + b.NoHistory = noHistory + return b, nil +} + // Get retrieves a bead by ID via bd show. func (s *BdStore) Get(id string) (Bead, error) { // Read via the transient-retry wrapper so a Get that races a managed-Dolt diff --git a/internal/beads/caching_store_writes.go b/internal/beads/caching_store_writes.go index a1cfe9770c..263a08cf71 100644 --- a/internal/beads/caching_store_writes.go +++ b/internal/beads/caching_store_writes.go @@ -15,14 +15,28 @@ func (c *CachingStore) Create(b Bead) (Bead, error) { // CreateWithStorage passes through a policy-selected storage class to backing // stores that support table-specific creates, then updates the cache. +// +// A backing store without the optional StorageCreateStore capability does NOT +// cause the class to be discarded: the class is stamped onto the bead's own +// Ephemeral/NoHistory fields and the plain Create carries it. Every storage +// backend routes on those fields (the beads library sends Ephemeral or +// NoHistory issues to the dolt_ignore'd wisps table and skips DOLT_COMMIT for +// them), so the policy still lands. Dropping the class here instead — the +// previous behavior — silently created every session bead in the committed +// issues table on any deployment whose backend lacks the capability, costing +// one Dolt commit per create and per subsequent update (vp-ia76: 727 of 730 +// session beads in 24h on the live hq city). func (c *CachingStore) CreateWithStorage(b Bead, storage StorageClass) (Bead, error) { - storageBacking, ok := c.backing.(StorageCreateStore) - if !ok { - return c.Create(b) + if storageBacking, ok := c.backing.(StorageCreateStore); ok { + return c.createWith(func() (Bead, error) { + return storageBacking.CreateWithStorage(b, storage) + }) } - return c.createWith(func() (Bead, error) { - return storageBacking.CreateWithStorage(b, storage) - }) + staged, err := beadWithStorageClass(b, storage) + if err != nil { + return Bead{}, fmt.Errorf("caching store create: %w", err) + } + return c.Create(staged) } func (c *CachingStore) createWith(create func() (Bead, error)) (Bead, error) { diff --git a/internal/beads/native_dolt_storage_class_live_test.go b/internal/beads/native_dolt_storage_class_live_test.go new file mode 100644 index 0000000000..89afddbc49 --- /dev/null +++ b/internal/beads/native_dolt_storage_class_live_test.go @@ -0,0 +1,152 @@ +package beads + +import ( + "context" + "database/sql" + "fmt" + "os" + "testing" + + _ "github.com/go-sql-driver/mysql" +) + +// TestNativeDoltStoreCreateWithStorageRoutesToWispsLive is the end-to-end proof +// for vp-ia76 against a REAL Dolt sql-server and a REAL beads schema — not a +// fake storage. It measures the thing the epic is about: the Dolt commit count. +// +// It is opt-in because it needs a scratch server. Run it with: +// +// GC_NATIVE_WISP_PROBE_SCOPE= \ +// GC_NATIVE_WISP_PROBE_DSN='root@tcp(127.0.0.1:49991)/' \ +// GC_NATIVE_WISP_PROBE_DB= \ +// GC_NATIVE_WISP_PROBE_PORT=49991 \ +// go test ./internal/beads/ -run TestNativeDoltStoreCreateWithStorageRoutesToWispsLive -count=1 -v +// +// NEVER point it at the live fleet server. It writes beads. +func TestNativeDoltStoreCreateWithStorageRoutesToWispsLive(t *testing.T) { + scope := os.Getenv("GC_NATIVE_WISP_PROBE_SCOPE") + dsn := os.Getenv("GC_NATIVE_WISP_PROBE_DSN") + dbName := os.Getenv("GC_NATIVE_WISP_PROBE_DB") + port := os.Getenv("GC_NATIVE_WISP_PROBE_PORT") + if scope == "" || dsn == "" || dbName == "" || port == "" { + t.Skip("scratch Dolt server not configured; see the doc comment") + } + + db, err := sql.Open("mysql", dsn) + if err != nil { + t.Fatalf("open probe db: %v", err) + } + defer db.Close() //nolint:errcheck + + commits := func() int { + var n int + if err := db.QueryRow(fmt.Sprintf("select count(*) from `%s`.dolt_log", dbName)).Scan(&n); err != nil { + t.Fatalf("count dolt_log: %v", err) + } + return n + } + tableOf := func(id string) string { + var n int + if err := db.QueryRow(fmt.Sprintf("select count(*) from `%s`.issues where id = ?", dbName), id).Scan(&n); err != nil { + t.Fatalf("count issues: %v", err) + } + if n > 0 { + return "issues" + } + if err := db.QueryRow(fmt.Sprintf("select count(*) from `%s`.wisps where id = ?", dbName), id).Scan(&n); err != nil { + t.Fatalf("count wisps: %v", err) + } + if n > 0 { + return "wisps" + } + return "missing" + } + + ctx := context.Background() + storage, err := OpenNativeStorage(ctx, scope, map[string]string{ + "BEADS_DOLT_SERVER_MODE": "1", + "BEADS_DOLT_SERVER_HOST": "127.0.0.1", + "BEADS_DOLT_SERVER_PORT": port, + "BEADS_DOLT_SERVER_USER": "root", + "BEADS_DOLT_SERVER_DATABASE": dbName, + "BEADS_DOLT_SERVER_TLS": "false", + "BEADS_DOLT_AUTO_START": "0", + }) + if err != nil { + t.Fatalf("OpenNativeStorage: %v", err) + } + defer storage.Close() //nolint:errcheck + store := newNativeDoltStoreForTest(storage) + + sessionBead := func(title string) Bead { + return Bead{Title: title, Type: "session", Labels: []string{"gc:session", "agent:probe"}} + } + + // Control: the current behavior. A session bead created without a storage + // class lands in the committed issues table and costs one Dolt commit. + before := commits() + plain, err := store.Create(sessionBead("probe plain session")) + if err != nil { + t.Fatalf("Create: %v", err) + } + plainCost := commits() - before + if got := tableOf(plain.ID); got != "issues" { + t.Fatalf("plain session create landed in %s, want issues", got) + } + if plainCost != 1 { + t.Fatalf("plain session create cost %d Dolt commits, want 1", plainCost) + } + + // The fix: the same bead with the session policy's storage class must land + // in the dolt_ignore'd wisps table at zero commits. + before = commits() + wisp, err := store.CreateWithStorage(sessionBead("probe no-history session"), StorageNoHistory) + if err != nil { + t.Fatalf("CreateWithStorage: %v", err) + } + wispCost := commits() - before + if got := tableOf(wisp.ID); got != "wisps" { + t.Fatalf("no-history session create landed in %s, want wisps", got) + } + if wispCost != 0 { + t.Fatalf("no-history session create cost %d Dolt commits, want 0", wispCost) + } + + // The payload claim: updates are where most of hq's commit graph comes from + // (2,885 of 3,363 commits in a measured 6h window), so measure them, not + // only the create. + before = commits() + status := "in_progress" + for i := 0; i < 5; i++ { + if err := store.Update(plain.ID, UpdateOpts{Status: &status}); err != nil { + t.Fatalf("Update issues-table session: %v", err) + } + open := "open" + if err := store.Update(plain.ID, UpdateOpts{Status: &open}); err != nil { + t.Fatalf("Update issues-table session: %v", err) + } + } + issuesUpdateCost := commits() - before + + before = commits() + for i := 0; i < 5; i++ { + if err := store.Update(wisp.ID, UpdateOpts{Status: &status}); err != nil { + t.Fatalf("Update wisp session: %v", err) + } + open := "open" + if err := store.Update(wisp.ID, UpdateOpts{Status: &open}); err != nil { + t.Fatalf("Update wisp session: %v", err) + } + } + wispUpdateCost := commits() - before + + t.Logf("10 updates: issues-table session = %d commits, wisps-table session = %d commits", + issuesUpdateCost, wispUpdateCost) + if wispUpdateCost != 0 { + t.Fatalf("10 updates to a wisps-table session cost %d Dolt commits, want 0", wispUpdateCost) + } + if issuesUpdateCost == 0 { + t.Fatal("10 updates to an issues-table session cost 0 Dolt commits: " + + "the control is not exercising the committing path, so the comparison proves nothing") + } +} diff --git a/internal/beads/native_dolt_store.go b/internal/beads/native_dolt_store.go index a131d38c17..4df4dfa033 100644 --- a/internal/beads/native_dolt_store.go +++ b/internal/beads/native_dolt_store.go @@ -821,6 +821,23 @@ func (s *NativeDoltStore) SupportsEphemeralGraphApply() bool { return true } +// CreateWithStorage persists a new bead using a policy-selected storage tier. +// The tier is carried on the issue's own Ephemeral/NoHistory fields, which is +// what the upstream beads storage layer routes on: a no-history or ephemeral +// issue is created in the dolt_ignore'd wisps table and skips DOLT_COMMIT +// entirely (beads internal/storage/dolt/issues.go CreateIssue). Without this +// method the native store is not a StorageCreateStore, and the caching layer +// above it had no way to express the class — so every session bead was created +// in the committed issues table at a cost of one Dolt commit per create and one +// per subsequent update (vp-ia76). +func (s *NativeDoltStore) CreateWithStorage(b Bead, storage StorageClass) (Bead, error) { + staged, err := beadWithStorageClass(b, storage) + if err != nil { + return Bead{}, fmt.Errorf("native create: %w", err) + } + return s.Create(staged) +} + // Create persists a new bead through the upstream beads storage layer. func (s *NativeDoltStore) Create(b Bead) (Bead, error) { issue, err := nativeIssueFromBead(b) From 3f012b6e575b2e13be82cd08dd755fd6b45cd89d Mon Sep 17 00:00:00 2001 From: "voxist.executor" Date: Thu, 6 Aug 2026 15:35:39 +0000 Subject: [PATCH 3/4] =?UTF-8?q?fix(session):=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20per-type=20warning,=20policy-store=20marker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the PR #124 review, all verified before fixing: * THE ONCE-GUARD COULD BE BURNED BY A FALSE ALARM. warnSessionStorageUnsupported was once-per-process, and the first (benign) alarm for beadPolicyGraphStore consumed it — so a later, genuinely incapable chain of a DIFFERENT store type wrote sessions to the committed table with no warning at all: a silent failure inside the warning that exists to prevent silent failure. Now once per store TYPE (bounded noise, never mutes a new offender). * THE FALSE ALARM ITSELF. beadPolicyStore applies the session storage policy in its own Create (policyForCreate -> createWithStoragePolicy) but deliberately does not forward CreateWithStorage, so the front door's capability probe misread it as incapable. It now declares AppliesBeadStoragePolicy(), and the front door recognizes the structural marker and creates quietly through it — verified equivalent on the live fleet (27 sessions -> wisps, 0 -> issues, all via this wrapper). * STALE DOC: createSessionBead's comment still described CachingStore's OLD silently-degrading fallback, which commit 2 of this PR replaced with class-stamping. Reworded to past tense. Also assessed, no change: the new error returns on CachingStore's incapable fallback (both-flags-preset, unknown class) fire only on contradictory input the capable path already rejected — refusing loudly beats persisting garbage. A fourth finding in the original round-2 commit (deduplicating the init-test teardown onto cleanupManagedDoltTestCity) is dropped: PR #125 fixed the same init-test leak on main with the hermetic GC_BEADS/GC_DOLT idiom, and cmd/gc/init_from_hosted_dolt_test.go now matches main exactly. Mutations, each confirmed applied: dropping the self-applying branch -> 1 failure; reverting to a process-wide guard -> 1 failure. Suites: session, beads, and the original leak single-test all green. --- cmd/gc/bead_policy_store.go | 7 ++ internal/session/create.go | 63 +++++++++++++----- internal/session/create_storage_test.go | 87 ++++++++++++++++++++++++- 3 files changed, 138 insertions(+), 19 deletions(-) diff --git a/cmd/gc/bead_policy_store.go b/cmd/gc/bead_policy_store.go index 067aa97067..173d0fb643 100644 --- a/cmd/gc/bead_policy_store.go +++ b/cmd/gc/bead_policy_store.go @@ -50,6 +50,13 @@ var ( // *beadPolicyStore. func (s *beadPolicyStore) ConditionalWritesResolveTarget() beads.Store { return s.Store } +// AppliesBeadStoragePolicy marks this wrapper as applying the bead storage +// policy inside its own Create (policyForCreate -> createWithStoragePolicy), so +// callers that cannot reach CreateWithStorage through it — it is deliberately +// not forwarded — know the policy is still applied and need not warn. Consumed +// by internal/session's createSessionBead via a structural interface. +func (s *beadPolicyStore) AppliesBeadStoragePolicy() {} + var ( _ beads.BatchDeleter = (*beadPolicyStore)(nil) _ beads.BatchDeleter = (*beadPolicyGraphStore)(nil) diff --git a/internal/session/create.go b/internal/session/create.go index e73a3fd16a..263c4f8034 100644 --- a/internal/session/create.go +++ b/internal/session/create.go @@ -97,7 +97,7 @@ func (s *Store) CreateSessionInfo(spec CreateSpec) (Info, error) { // declares incompatible for sessions (bead_policy_store.go) and which // matchesTier (internal/beads/query.go) silently DROPS from results. // -// THE FALLBACK IS LOUD ON PURPOSE. CachingStore.CreateWithStorage silently degrades to +// THE FALLBACK IS LOUD ON PURPOSE. Before this PR, CachingStore.CreateWithStorage degraded to // a plain Create when its backing store does not implement StorageCreateStore — and // NativeDoltStore does not implement it. So a chain assembled the wrong way would take // this fix, report success, and keep writing to issues exactly as before: the fix would @@ -105,29 +105,58 @@ func (s *Store) CreateSessionInfo(spec CreateSpec) (Info, error) { // (ADR-0043: an unknown must propagate, not be coerced into the quiet answer). When the // class cannot be honored we say so rather than pretend, once per process so a hot path // cannot spam the ops tail. +// storagePolicySelfApplying marks a store wrapper that resolves and applies the +// bead storage policy inside its own Create — so a caller that cannot reach +// CreateWithStorage through it has NOT lost the policy. The policy layer in +// cmd/gc declares this; see beadPolicyStore.AppliesBeadStoragePolicy. Review +// finding (PR #124): without this, the front door warned on every boot for the +// policy wrapper — a false alarm that then BURNED the once-guard, so a later, +// genuinely incapable chain in the same process failed silently. +type storagePolicySelfApplying interface { + AppliesBeadStoragePolicy() +} + func (s *Store) createSessionBead(b beads.Bead) (beads.Bead, error) { - storageStore, ok := s.store.Store.(beads.StorageCreateStore) - if !ok { - warnSessionStorageUnsupported(s.store.Store) + if storageStore, ok := s.store.Store.(beads.StorageCreateStore); ok { + return storageStore.CreateWithStorage(b, beads.StorageNoHistory) + } + if _, ok := s.store.Store.(storagePolicySelfApplying); ok { + // The wrapper applies the session storage policy itself; a plain Create + // through it still lands the bead in wisps. Verified on the live fleet + // 2026-08-06: 27 sessions -> wisps, 0 -> issues, all through this path. return s.store.Create(b) } - return storageStore.CreateWithStorage(b, beads.StorageNoHistory) + warnSessionStorageUnsupported(s.store.Store) + return s.store.Create(b) } -var sessionStorageWarnOnce sync.Once +var ( + sessionStorageWarnMu sync.Mutex + sessionStorageWarnTypes = map[string]bool{} +) -// warnSessionStorageUnsupported reports, once per process, that the session storage -// policy could not be applied. Silence here would mean the caller believes sessions are -// staying out of the committed table while they are not. +// warnSessionStorageUnsupported reports, once per process PER STORE TYPE, that +// the session storage policy could not be applied. Once-per-process was wrong: +// a single benign false alarm burned the guard, and a later genuinely +// incapable chain of a DIFFERENT type then wrote sessions to the committed +// table with no warning at all — a silent failure inside the warning that +// exists to prevent silent failure. Per-type keeps the noise bounded (one line +// per offending type per process) without ever muting a new offender. func warnSessionStorageUnsupported(store any) { - sessionStorageWarnOnce.Do(func() { - fmt.Fprintf(os.Stderr, - "gc: session storage policy NOT applied: %T does not implement "+ - "beads.StorageCreateStore, so session beads are being written to the "+ - "committed issues table (one DOLT_COMMIT each) instead of wisps. "+ - "This is vp-ia76 / vp-9u1 and it silently inflates the store.\n", - store) - }) + typeName := fmt.Sprintf("%T", store) + sessionStorageWarnMu.Lock() + seen := sessionStorageWarnTypes[typeName] + sessionStorageWarnTypes[typeName] = true + sessionStorageWarnMu.Unlock() + if seen { + return + } + fmt.Fprintf(os.Stderr, + "gc: session storage policy NOT applied: %s does not implement "+ + "beads.StorageCreateStore, so session beads are being written to the "+ + "committed issues table (one DOLT_COMMIT each) instead of wisps. "+ + "This is vp-ia76 / vp-9u1 and it silently inflates the store.\n", + typeName) } // CreateSession creates a session bead from spec and returns its id. It is the diff --git a/internal/session/create_storage_test.go b/internal/session/create_storage_test.go index 1ff1dd5141..9e56c3b889 100644 --- a/internal/session/create_storage_test.go +++ b/internal/session/create_storage_test.go @@ -5,7 +5,6 @@ import ( "io" "os" "strings" - "sync" "testing" "github.com/gastownhall/gascity/internal/beads" @@ -111,7 +110,9 @@ func TestUnsupportedStorageStillCreatesButIsReported(t *testing.T) { t.Fatalf("pipe: %v", err) } os.Stderr = w - sessionStorageWarnOnce = sync.Once{} + sessionStorageWarnMu.Lock() + sessionStorageWarnTypes = map[string]bool{} + sessionStorageWarnMu.Unlock() _, createErr := s.CreateSessionInfo(newSpec()) @@ -142,3 +143,85 @@ func TestUnsupportedStorageStillCreatesButIsReported(t *testing.T) { "cannot tell which chain dropped the policy. got: %q", buf.String()) } } + +// policySelfApplyingStore mimics cmd/gc's beadPolicyStore: no CreateWithStorage, +// but declares (and performs) its own policy application on Create. +type policySelfApplyingStore struct { + beads.Store + plainCreates int +} + +func (p *policySelfApplyingStore) Create(b beads.Bead) (beads.Bead, error) { + p.plainCreates++ + return b, nil +} +func (p *policySelfApplyingStore) AppliesBeadStoragePolicy() {} + +// A policy-self-applying wrapper must create WITHOUT any warning: warning here +// was the false alarm that burned the old process-wide once-guard. +func TestPolicySelfApplyingStoreCreatesQuietly(t *testing.T) { + captureStderr := func(fn func()) string { + old := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + os.Stderr = w + fn() + if err := w.Close(); err != nil { + t.Fatalf("close: %v", err) + } + os.Stderr = old + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("copy: %v", err) + } + return buf.String() + } + sessionStorageWarnMu.Lock() + sessionStorageWarnTypes = map[string]bool{} + sessionStorageWarnMu.Unlock() + + pol := &policySelfApplyingStore{} + s := NewStore(beads.SessionStore{Store: pol}) + out := captureStderr(func() { + if _, err := s.CreateSessionInfo(newSpec()); err != nil { + t.Fatalf("CreateSessionInfo: %v", err) + } + }) + if pol.plainCreates != 1 { + t.Errorf("plain creates = %d, want 1", pol.plainCreates) + } + if strings.Contains(out, "NOT applied") { + t.Errorf("policy-self-applying store must not warn; got %q", out) + } +} + +// One benign incapable type must not mute the warning for a DIFFERENT +// incapable type later in the same process (the burned-once-guard defect). +func TestWarnIsPerStoreTypeNotPerProcess(t *testing.T) { + sessionStorageWarnMu.Lock() + sessionStorageWarnTypes = map[string]bool{} + sessionStorageWarnMu.Unlock() + + warnSessionStorageUnsupported(&plainOnlyStore{}) + old := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + os.Stderr = w + warnSessionStorageUnsupported(&policySelfApplyingStore{}) // different type: must warn + warnSessionStorageUnsupported(&plainOnlyStore{}) // repeat type: must stay quiet + if err := w.Close(); err != nil { + t.Fatalf("close: %v", err) + } + os.Stderr = old + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("copy: %v", err) + } + if n := strings.Count(buf.String(), "NOT applied"); n != 1 { + t.Errorf("want exactly 1 warning for the new type (repeat muted), got %d: %q", n, buf.String()) + } +} From 2b6fa919bf0f4d46731f8171219c772a5ffd34eb Mon Sep 17 00:00:00 2001 From: "voxist.executor" Date: Tue, 11 Aug 2026 16:30:49 +0000 Subject: [PATCH 4/4] =?UTF-8?q?fix(session,beads):=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20a=20marker=20guard=20that=20can=20fail,=20storage-c?= =?UTF-8?q?lass=20test,=20stamping=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight findings from the merge-set review. Every guard was mutation-verified against the file it protects; the mutation was confirmed present before running. * THE MARKER CONTRACT HAD NO GUARD THAT COULD GO RED (finding 1). Renaming beadPolicyStore.AppliesBeadStoragePolicy, or internal/session's consumer, left the whole suite green: the front door just stopped recognizing the wrapper and fell through to the warn-and-stamp path, which still creates the bead with the same class by coincidence of the default. Two halves now fail: the interface is exported as session.StoragePolicySelfApplying and pinned in cmd/gc with `var _ session.StoragePolicySelfApplying = (*beadPolicyStore)(nil)` (a rename on either side is a build failure), and TestPolicyStoreCompositionCreatesSessionsSilently drives a create through the REAL wrapStoreWithBeadPolicies + wrapWithCachingStore composition and asserts stderr silence. That assertion resets the process-wide warn ledger first (session.ResetStorageWarningsForTest) — without the reset a sibling test burns the ledger for the same store type and the silence assertion passes for the wrong reason, which is the defect this whole finding is about. * NativeDoltStore.CreateWithStorage HAD NO ALWAYS-ON BEHAVIORAL TEST (finding 2). Its only coverage was the opt-in live probe. TestNativeDoltStoreCreateWithStorageStampsClass runs the real method body against newNativeDoltStoreForTest(newNativeDoltMemStorage()) and asserts the created AND persisted bead carries the class for all four classes, plus that an unknown class is refused without persisting anything. * THE WARN-AND-DROP FALLBACK NOW STAMPS (finding 11). createSessionBead's last resort warned and then created with no class at all — the same field routing the caching store's own fallback relies on costs nothing, so it sets NoHistory before Create. The warning stays. * THE WARNING WAS DOLT-SPECIFIC (finding 4). It asserted "committed issues table (one DOLT_COMMIT each)", which is false for the FileStore and MemStore chains that reach the same path. It now names the store type and the policy that was lost. * TEST HYGIENE (finding 5). The stderr swaps in create_storage_test.go restored os.Stderr sequentially, so a t.Fatalf inside a capture stranded the whole test binary on an orphaned pipe; captureStderr now restores via defer. And TestWarnIsPerStoreTypeNotPerProcess printed its first, real warning to the real stderr before redirecting — it captures from the first call. * COMMENT ALTITUDE (findings 13/14). The 35-line essay was attached to the marker type, leaving createSessionBead undocumented; it still said "once per process" (now per-type) and narrated PR history ("Before this PR...", "Review finding (PR #124)..."). Rewritten to state the current contract at the symbol that owns it. * CAPABILITY IDIOM (finding 12). `_ StorageCreateStore = (*NativeDoltStore)(nil)` joins the existing compile-time var block; the runtime TestNativeDoltStoreDeclaresStorageCreateCapability re-check is deleted. * LIVE PROBE (finding 8). Tagged `//go:build integration` per AGENTS.md, matching bdstore_conditional_integration_test.go. GC_NATIVE_WISP_PROBE_DB reaches the probe queries as an SQL identifier, where a placeholder cannot bind, so it is validated against a strict `^[A-Za-z0-9_]+$` whitelist first. Its stale claim that the native store lacks CreateWithStorage is gone. Two PLAUSIBLE findings assessed: * THE FRONT DOOR'S HARDCODED CLASS (9a) — FIXED CHEAPLY. The StoragePolicySelfApplying probe now runs BEFORE the StorageCreateStore probe, so a store that resolves the CONFIGURED class wins over the front door's hardcoded StorageNoHistory. No store implements both today, so behavior is unchanged; the ordering makes the marker authoritative and bounds the divergence, which is documented at the call site. * THE STAMPING FALLBACK'S BLAST RADIUS (9b) — DOCUMENTED, NOT CHANGED. It activates classes for all six policy names on previously-incapable backings, not just sessions. session/wait/nudge/order_tracking gain no_history, which does not change query visibility (matchesTier only filters on Ephemeral). wisp gains EPHEMERAL under bd-105 ready semantics, which IS GC/TTL-eligible and IS dropped by a default-tier read — safe through the policy layer (it expands TierIssues to TierBoth), visible to a raw un-wrapped read. Capable backings (BdStore) already behaved this way, so this converges incapable ones rather than inventing new behavior. Written into the caching-store comment. Mutations, each confirmed present in the file before running: rename beadPolicyStore.AppliesBeadStoragePolicy -> build failure rename the StoragePolicySelfApplying method -> build failure drop the marker branch in createSessionBead -> 3 failures NativeDoltStore.CreateWithStorage: Create(staged)->Create(b) -> 1 failure (3 subtests) drop the NoHistory stamp in the last resort -> 1 failure restore the Dolt-specific warning text -> 1 failure put StorageCreateStore probe back ahead of the marker -> 1 failure rename NativeDoltStore.CreateWithStorage -> build failure regress the per-type ledger to per-process -> 1 failure unanchor the probe-name regexp -> 1 failure CachingStore fallback: Create(staged)->Create(b) -> 1 failure Not mutation-verified, triaged as no-op-under-green: captureStderr's deferred restore has no observable effect while every assertion passes — it only changes what happens after a failure — so there is no guard to write for it. --- cmd/gc/bead_policy_store.go | 19 +- cmd/gc/session_storage_policy_wiring_test.go | 102 +++++-- internal/beads/caching_store_writes.go | 24 ++ .../native_dolt_storage_class_live_test.go | 55 +++- internal/beads/native_dolt_store.go | 1 + .../native_dolt_store_storage_class_test.go | 100 +++++++ internal/session/create.go | 145 ++++++---- internal/session/create_storage_test.go | 259 ++++++++++++------ 8 files changed, 535 insertions(+), 170 deletions(-) create mode 100644 internal/beads/native_dolt_store_storage_class_test.go diff --git a/cmd/gc/bead_policy_store.go b/cmd/gc/bead_policy_store.go index 173d0fb643..33dfda2117 100644 --- a/cmd/gc/bead_policy_store.go +++ b/cmd/gc/bead_policy_store.go @@ -38,6 +38,15 @@ type beadPolicyGraphStore struct { var ( _ beads.ConditionalAssignmentReleaser = (*beadPolicyStore)(nil) _ beads.ConditionalWritesResolveTargeter = (*beadPolicyStore)(nil) + + // The session front door recognizes this wrapper structurally: without the + // marker it cannot tell a policy-applying store from a store that dropped + // the policy, and it warns and stamps its own hardcoded class instead of + // deferring to the configured one. A structural interface has no compiler + // coupling of its own, so state it here — renaming either side is then a + // build failure, not a silent behavior change. + _ session.StoragePolicySelfApplying = (*beadPolicyStore)(nil) + _ session.StoragePolicySelfApplying = (*beadPolicyGraphStore)(nil) ) // ConditionalWritesResolveTarget declares the wrapped store as the @@ -51,10 +60,12 @@ var ( func (s *beadPolicyStore) ConditionalWritesResolveTarget() beads.Store { return s.Store } // AppliesBeadStoragePolicy marks this wrapper as applying the bead storage -// policy inside its own Create (policyForCreate -> createWithStoragePolicy), so -// callers that cannot reach CreateWithStorage through it — it is deliberately -// not forwarded — know the policy is still applied and need not warn. Consumed -// by internal/session's createSessionBead via a structural interface. +// policy inside its own Create (policyForCreate -> createWithStoragePolicy). +// CreateWithStorage is deliberately not forwarded — an out-of-band class from a +// caller would override the class this layer resolves from city config — so a +// caller that finds no beads.StorageCreateStore here has NOT lost the policy. +// It implements session.StoragePolicySelfApplying; see the compile-time +// assertion above. func (s *beadPolicyStore) AppliesBeadStoragePolicy() {} var ( diff --git a/cmd/gc/session_storage_policy_wiring_test.go b/cmd/gc/session_storage_policy_wiring_test.go index 0d7c97ef8e..a831daa54b 100644 --- a/cmd/gc/session_storage_policy_wiring_test.go +++ b/cmd/gc/session_storage_policy_wiring_test.go @@ -1,7 +1,11 @@ package main import ( + "bytes" "context" + "io" + "os" + "strings" "testing" "github.com/gastownhall/gascity/internal/beads" @@ -30,11 +34,9 @@ func (s *capableBackingStore) CreateWithStorage(b beads.Bead, storage beads.Stor } // incapableBackingStore is a backend that does NOT implement -// StorageCreateStore. This is the shape *beads.NativeDoltStore has today: it -// implements Create (and ApplyGraphPlanWithStorage) but no single-bead -// CreateWithStorage. It is the store the live hq city runs on — proven by the -// "gc: update bead " commit messages in hq.dolt_log, which are emitted only -// by NativeDoltStore.Update (internal/beads/native_dolt_store.go:1004). +// StorageCreateStore — the shape *beads.NativeDoltStore had before this change, +// and the shape any future or third-party backend may have, since the +// capability is optional by design. // // It records the bead exactly as it arrives, so the test can see whether the // no-history storage class survived the trip. Routing to the wisps table is @@ -99,11 +101,10 @@ func TestSessionCreateRoutesToNoHistoryOnCapableBackend(t *testing.T) { // TestSessionCreateRoutesToNoHistoryOnIncapableBackend is the vp-ia76 guard. // // A backend that cannot honor a storage class must not cause the class to be -// discarded. CachingStore.CreateWithStorage currently falls back to plain -// Create when the backing store is not a StorageCreateStore -// (internal/beads/caching_store_writes.go:20-22), dropping the policy silently -// — no error, no warning, no signal of any kind. That is ADR-0043 Cause 1: -// an unsupported capability coerced into the quiet default. +// discarded: CachingStore.CreateWithStorage stamps it onto the bead's own +// fields instead (internal/beads/caching_store_writes.go). Discarding it would +// be ADR-0043 Cause 1 — an unsupported capability coerced into the quiet +// default, with no error, no warning, and no signal of any kind. // // The consequence is measurable on the live fleet, not theoretical. Measured on // hq 2026-07-31: 727 of 730 session beads created in 24h landed in the @@ -113,9 +114,8 @@ func TestSessionCreateRoutesToNoHistoryOnCapableBackend(t *testing.T) { // the fleet's configuration): a session bead in issues costs one Dolt commit on // create and one on every update; the same bead in wisps costs zero for both. // -// Reverting the fix must turn this red. It asserts the bead the backend -// actually receives — the value the beads library routes on — not that some -// wrapper was called. +// It asserts the bead the backend actually receives — the value the beads +// library routes on — not that some wrapper was called. func TestSessionCreateRoutesToNoHistoryOnIncapableBackend(t *testing.T) { backing := &incapableBackingStore{Store: beads.NewMemStore()} @@ -134,14 +134,72 @@ func TestSessionCreateRoutesToNoHistoryOnIncapableBackend(t *testing.T) { } } -// TestNativeDoltStoreDeclaresStorageCreateCapability pins the capability -// itself. The live city store is a *beads.NativeDoltStore; without this -// assertion the drop above is reachable again the moment the fallback is -// touched. -func TestNativeDoltStoreDeclaresStorageCreateCapability(t *testing.T) { - var store any = (*beads.NativeDoltStore)(nil) - if _, ok := store.(beads.StorageCreateStore); !ok { - t.Fatal("*beads.NativeDoltStore does not implement beads.StorageCreateStore: " + - "policy-selected storage classes are silently discarded on the store the live city runs on") +// TestPolicyStoreCompositionCreatesSessionsSilently is the guard for the +// storage-policy MARKER contract. +// +// internal/session recognizes cmd/gc's policy wrapper structurally: if the +// marker method disappears from either side, the front door stops recognizing +// the wrapper and falls through to the last-resort path, which warns on stderr +// and imposes its own hardcoded class over the configured one. Nothing else +// fails — the bead is still created, the storage class still ends up +// no_history by coincidence of the default, and every other test in this +// package stays green. The observable symptom is the warning, so that is what +// this asserts: stderr SILENCE through the REAL composition +// (wrapStoreWithBeadPolicies + wrapWithCachingStore + the session front door). +// +// Its compile-time half lives in bead_policy_store.go +// (`var _ session.StoragePolicySelfApplying = (*beadPolicyStore)(nil)`), which +// catches a rename on either side at build time. This catches the wiring: a +// composition that never puts the marked wrapper where the front door looks. +func TestPolicyStoreCompositionCreatesSessionsSilently(t *testing.T) { + // The warning ledger is process-wide and keyed by store TYPE, and the + // sibling tests above create sessions through this very type. Without the + // reset, a broken marker contract would warn once for them and then stay + // quiet here — this guard would pass for the wrong reason. + session.ResetStorageWarningsForTest() + t.Cleanup(session.ResetStorageWarningsForTest) + + backing := &incapableBackingStore{Store: beads.NewMemStore()} + store := controllerCityStore(t, backing) + + if _, ok := store.(session.StoragePolicySelfApplying); !ok { + t.Fatalf("the controller's session store composition is %T, which the session "+ + "front door cannot recognize as policy-self-applying; it will warn and "+ + "impose its own storage class instead of the configured one", store) + } + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + defer r.Close() //nolint:errcheck + old := os.Stderr + os.Stderr = w + restored := false + restore := func() { + if restored { + return + } + restored = true + os.Stderr = old + w.Close() //nolint:errcheck + } + // Deferred, so a t.Fatalf below cannot strand os.Stderr on a dead pipe. + defer restore() + + if _, err := sessionFrontDoor(store).CreateSessionInfo(sessionCreateSpec()); err != nil { + t.Fatalf("CreateSessionInfo: %v", err) + } + + restore() + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("read captured stderr: %v", err) + } + if strings.Contains(buf.String(), "session storage policy NOT applied") { + t.Fatalf("creating a session through the real policy composition warned that the "+ + "storage policy was not applied; the marker contract between "+ + "beadPolicyStore.AppliesBeadStoragePolicy and "+ + "session.StoragePolicySelfApplying is broken. got: %q", buf.String()) } } diff --git a/internal/beads/caching_store_writes.go b/internal/beads/caching_store_writes.go index 263a08cf71..6d5982748c 100644 --- a/internal/beads/caching_store_writes.go +++ b/internal/beads/caching_store_writes.go @@ -26,6 +26,30 @@ func (c *CachingStore) Create(b Bead) (Bead, error) { // issues table on any deployment whose backend lacks the capability, costing // one Dolt commit per create and per subsequent update (vp-ia76: 727 of 730 // session beads in 24h on the live hq city). +// +// BLAST RADIUS. This is not a session-only change. cmd/gc's policy layer routes +// six policy names through CreateWithStorage (bead_policy_store.go: +// policyNameForBead -> effectiveBeadStorage), so on a backing that lacks the +// capability EVERY one of them starts carrying its class where it previously +// carried none: +// +// - session, wait, nudge, order_tracking: no_history under both semantics. +// NoHistory does not change query visibility (ListQuery.matchesTier only +// filters on Ephemeral), so these gain wisps-table routing and lose nothing. +// - workflow: no_history under bd-105 ready semantics, history otherwise. +// - wisp: EPHEMERAL under bd-105 ready semantics. This is the one to look at +// twice — an ephemeral bead is GC/TTL-eligible AND is dropped by the default +// TierIssues read. Reads through the policy layer are safe (it expands +// TierIssues to TierBoth), but a raw un-wrapped read of the same store now +// misses newly created wisps on such a deployment. +// +// A capable backing (BdStore) already behaved this way — the class was forwarded +// and honored — so this converges incapable backings onto the behavior capable +// ones already had, rather than inventing a new one. +// +// An explicit class also OVERRIDES fields the caller stamped by hand: +// StorageHistory clears Ephemeral/NoHistory. Only StorageDefault leaves the +// incoming bead alone. func (c *CachingStore) CreateWithStorage(b Bead, storage StorageClass) (Bead, error) { if storageBacking, ok := c.backing.(StorageCreateStore); ok { return c.createWith(func() (Bead, error) { diff --git a/internal/beads/native_dolt_storage_class_live_test.go b/internal/beads/native_dolt_storage_class_live_test.go index 89afddbc49..c7982c0904 100644 --- a/internal/beads/native_dolt_storage_class_live_test.go +++ b/internal/beads/native_dolt_storage_class_live_test.go @@ -1,3 +1,5 @@ +//go:build integration + package beads import ( @@ -5,22 +7,67 @@ import ( "database/sql" "fmt" "os" + "regexp" "testing" _ "github.com/go-sql-driver/mysql" ) +// probeDBNamePattern constrains the probe database name to characters that are +// safe to interpolate into an SQL identifier. The name reaches the query as an +// identifier, where a placeholder cannot be used, so it is validated instead of +// escaped — a whitelist is the sound fix, quoting is not. +var probeDBNamePattern = regexp.MustCompile(`^[A-Za-z0-9_]+$`) + +// The identifier whitelist must actually reject the shapes that would let an +// operator-supplied database name change the meaning of the probe's queries. +// It needs no server, so it runs whenever the integration tag is on. +func TestProbeDBNamePatternRejectsSQLIdentifierInjection(t *testing.T) { + for _, ok := range []string{"gc", "hq_scratch", "probe1", "A_1"} { + if !probeDBNamePattern.MatchString(ok) { + t.Errorf("probeDBNamePattern rejected the plain identifier %q", ok) + } + } + for _, bad := range []string{ + "", + "hq`.issues; drop database hq; -- ", + "hq`", + "hq.issues", + "hq issues", + "hq-issues", + "hq\nissues", + "hq'", + } { + if probeDBNamePattern.MatchString(bad) { + t.Errorf("probeDBNamePattern accepted %q, which is not a bare SQL identifier "+ + "and would be interpolated into the probe queries verbatim", bad) + } + } +} + // TestNativeDoltStoreCreateWithStorageRoutesToWispsLive is the end-to-end proof // for vp-ia76 against a REAL Dolt sql-server and a REAL beads schema — not a // fake storage. It measures the thing the epic is about: the Dolt commit count. // -// It is opt-in because it needs a scratch server. Run it with: +// It is opt-in twice over: the `integration` build tag keeps it out of the +// default suite (AGENTS.md, "Integration tests use //go:build integration"), +// and it still skips unless a scratch server is configured. Run it with: // // GC_NATIVE_WISP_PROBE_SCOPE= \ // GC_NATIVE_WISP_PROBE_DSN='root@tcp(127.0.0.1:49991)/' \ // GC_NATIVE_WISP_PROBE_DB= \ // GC_NATIVE_WISP_PROBE_PORT=49991 \ -// go test ./internal/beads/ -run TestNativeDoltStoreCreateWithStorageRoutesToWispsLive -count=1 -v +// go test -tags integration ./internal/beads/ \ +// -run TestNativeDoltStoreCreateWithStorageRoutesToWispsLive -count=1 -v +// +// GC_NATIVE_WISP_PROBE_DB is interpolated into the probe queries as an SQL +// identifier (placeholders bind values, not identifiers), so it is validated +// against probeDBNamePattern first. +// +// The always-on sibling is TestNativeDoltStoreCreateWithStorageStampsClass, +// which pins the field routing against the in-memory storage fixture. This one +// exists for the part a fixture cannot show: the real table the row lands in +// and the Dolt commit it does or does not cost. // // NEVER point it at the live fleet server. It writes beads. func TestNativeDoltStoreCreateWithStorageRoutesToWispsLive(t *testing.T) { @@ -31,6 +78,10 @@ func TestNativeDoltStoreCreateWithStorageRoutesToWispsLive(t *testing.T) { if scope == "" || dsn == "" || dbName == "" || port == "" { t.Skip("scratch Dolt server not configured; see the doc comment") } + if !probeDBNamePattern.MatchString(dbName) { + t.Fatalf("GC_NATIVE_WISP_PROBE_DB = %q is not a plain [A-Za-z0-9_] identifier; "+ + "it is interpolated into the probe queries as an SQL identifier", dbName) + } db, err := sql.Open("mysql", dsn) if err != nil { diff --git a/internal/beads/native_dolt_store.go b/internal/beads/native_dolt_store.go index 4df4dfa033..eeb3a802b6 100644 --- a/internal/beads/native_dolt_store.go +++ b/internal/beads/native_dolt_store.go @@ -245,6 +245,7 @@ var ( _ AtomicTxStore = (*NativeDoltStore)(nil) _ GraphApplyStore = (*NativeDoltStore)(nil) _ StorageGraphApplyStore = (*NativeDoltStore)(nil) + _ StorageCreateStore = (*NativeDoltStore)(nil) _ EphemeralGraphApplyStore = (*NativeDoltStore)(nil) _ conditionalWritesModeCarrier = (*NativeDoltStore)(nil) ) diff --git a/internal/beads/native_dolt_store_storage_class_test.go b/internal/beads/native_dolt_store_storage_class_test.go new file mode 100644 index 0000000000..77585b009f --- /dev/null +++ b/internal/beads/native_dolt_store_storage_class_test.go @@ -0,0 +1,100 @@ +package beads + +import "testing" + +// The storage class a caller passes to CreateWithStorage must reach the created +// bead as the Ephemeral/NoHistory fields the storage layer routes on. This runs +// the REAL method body against the native store's in-memory storage fixture, so +// it is always-on: no scratch Dolt server, no build tag. The live probe in +// native_dolt_storage_class_live_test.go measures the resulting Dolt commit +// cost; this one pins the routing itself. +func TestNativeDoltStoreCreateWithStorageStampsClass(t *testing.T) { + cases := []struct { + name string + in Bead + storage StorageClass + wantEphemeral bool + wantNoHistory bool + }{ + { + // The session policy's class (vp-ia76): no_history, never ephemeral. + name: "no history", + in: Bead{Title: "native storage class session", Type: "session"}, + storage: StorageNoHistory, + wantNoHistory: true, + }, + { + // The wisp policy's class under bd-105 ready semantics. + name: "ephemeral", + in: Bead{Title: "native storage class wisp", Type: "wisp"}, + storage: StorageEphemeral, + wantEphemeral: true, + }, + { + // An explicit history class must CLEAR a class the caller stamped by + // hand: the policy decides the tier, not the incoming bead. + name: "history overrides incoming fields", + in: Bead{Title: "native storage class history", NoHistory: true}, + storage: StorageHistory, + }, + { + // StorageDefault is "no opinion": the bead's own fields survive. + name: "default preserves incoming fields", + in: Bead{Title: "native storage class default", NoHistory: true}, + storage: StorageDefault, + wantNoHistory: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := newNativeDoltStoreForTest(newNativeDoltMemStorage()) + + created, err := store.CreateWithStorage(tc.in, tc.storage) + if err != nil { + t.Fatalf("CreateWithStorage(%q): %v", tc.storage, err) + } + if created.Ephemeral != tc.wantEphemeral { + t.Errorf("created.Ephemeral = %v, want %v: storage class %q did not reach "+ + "the created bead, so the storage layer routes it to the wrong tier", + created.Ephemeral, tc.wantEphemeral, tc.storage) + } + if created.NoHistory != tc.wantNoHistory { + t.Errorf("created.NoHistory = %v, want %v: storage class %q did not reach "+ + "the created bead, so the storage layer routes it to the wrong tier", + created.NoHistory, tc.wantNoHistory, tc.storage) + } + + // The class must be PERSISTED, not just echoed back by the create. + got, err := store.Get(created.ID) + if err != nil { + t.Fatalf("Get: %v", err) + } + if got.Ephemeral != tc.wantEphemeral { + t.Errorf("persisted Ephemeral = %v, want %v", got.Ephemeral, tc.wantEphemeral) + } + if got.NoHistory != tc.wantNoHistory { + t.Errorf("persisted NoHistory = %v, want %v", got.NoHistory, tc.wantNoHistory) + } + }) + } +} + +// An unknown class must be refused, not coerced into the quiet default +// (ADR-0043): a typo'd class that silently created a committed bead is exactly +// the failure this whole change exists to end. No bead may be persisted. +func TestNativeDoltStoreCreateWithStorageRejectsUnknownClass(t *testing.T) { + storage := newNativeDoltMemStorage() + store := newNativeDoltStoreForTest(storage) + + if _, err := store.CreateWithStorage(Bead{Title: "bogus class"}, StorageClass("nonsense")); err == nil { + t.Fatal("CreateWithStorage with an unknown storage class returned no error") + } + beads, err := storage.store.List(ListQuery{AllowScan: true, IncludeClosed: true, TierMode: TierBoth}) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(beads) != 0 { + t.Fatalf("unknown storage class persisted %d bead(s), want 0", len(beads)) + } +} diff --git a/internal/session/create.go b/internal/session/create.go index 263c4f8034..0c8a02531d 100644 --- a/internal/session/create.go +++ b/internal/session/create.go @@ -77,56 +77,75 @@ func (s *Store) CreateSessionInfo(spec CreateSpec) (Info, error) { return infoFromPersistedBead(created), nil } -// createSessionBead persists a session bead under the SESSION STORAGE POLICY. +// StoragePolicySelfApplying marks a store that resolves and applies the bead +// storage policy inside its own Create. A caller that cannot reach +// CreateWithStorage through such a store has NOT lost the policy, and must not +// warn: the plain Create is already policy-correct. // -// WHY THIS EXISTS (vp-ia76, phase 1 of vp-9u1). This front door used to call -// s.store.Create() directly, which carries no storage class. gascity's own policy -// (cmd/gc/bead_policy_store.go: beadPolicySession -> beadStorageNoHistory) was -// therefore dropped on the floor, and every session bead landed in the Dolt-COMMITTED -// issues table instead of the dolt_ignore'd wisps table — one DOLT_COMMIT each. -// Measured 2026-07-30: 262 sessions/24h through this door into issues, against 110 -// into wisps through the policy-honoring controller path. That commit volume is what -// grows the hq store, drives compaction, and rebuilds the push backlog faster than the -// 15s listener window can ship it. +// It is also the only party that can see a CONFIGURED policy — the storage class +// for a policy name is read from the city config +// ([beads.policies.session] storage = ...), which internal/session has no access +// to. So a store that declares this marker is authoritative about the session +// bead's storage class, and createSessionBead defers to it. // -// no_history, NOT ephemeral. They are not interchangeable: -// - no_history: row in wisps, ephemeral=0, no DOLT_COMMIT, NOT GC/TTL-eligible, -// reads keep working. The 204 session rows already in hq.wisps have exactly this -// shape (no_history=1, ephemeral=0) — this matches them. -// - ephemeral: also GC/TTL-eligible, sets ephemeral=1, which gascity's own policy -// declares incompatible for sessions (bead_policy_store.go) and which -// matchesTier (internal/beads/query.go) silently DROPS from results. -// -// THE FALLBACK IS LOUD ON PURPOSE. Before this PR, CachingStore.CreateWithStorage degraded to -// a plain Create when its backing store does not implement StorageCreateStore — and -// NativeDoltStore does not implement it. So a chain assembled the wrong way would take -// this fix, report success, and keep writing to issues exactly as before: the fix would -// be INERT AND INVISIBLE, which is the failure mode this change exists to end -// (ADR-0043: an unknown must propagate, not be coerced into the quiet answer). When the -// class cannot be honored we say so rather than pretend, once per process so a hot path -// cannot spam the ops tail. -// storagePolicySelfApplying marks a store wrapper that resolves and applies the -// bead storage policy inside its own Create — so a caller that cannot reach -// CreateWithStorage through it has NOT lost the policy. The policy layer in -// cmd/gc declares this; see beadPolicyStore.AppliesBeadStoragePolicy. Review -// finding (PR #124): without this, the front door warned on every boot for the -// policy wrapper — a false alarm that then BURNED the once-guard, so a later, -// genuinely incapable chain in the same process failed silently. -type storagePolicySelfApplying interface { +// cmd/gc's policy layer is the implementation; the coupling is pinned there by +// `var _ session.StoragePolicySelfApplying = (*beadPolicyStore)(nil)`, so +// renaming either side is a compile error rather than a silent downgrade to the +// warn path. +type StoragePolicySelfApplying interface { AppliesBeadStoragePolicy() } +// createSessionBead persists a session bead under the SESSION STORAGE POLICY: +// no_history, never ephemeral. +// +// no_history and ephemeral are NOT interchangeable, and picking the wrong one +// is worse than picking neither: +// - no_history sets no_history=1, ephemeral=0. Retention is dropped, nothing +// else is: the bead is NOT GC/TTL-eligible and reads keep finding it. (On +// the Dolt backend this is the dolt_ignore'd wisps table, at zero +// DOLT_COMMITs; other backends express the same tier their own way.) +// - ephemeral sets ephemeral=1, which is ALSO GC/TTL-eligible, which +// gascity's own policy declares incompatible for sessions +// (cmd/gc/bead_policy_store.go), and which ListQuery.matchesTier +// (internal/beads/query.go) silently DROPS from default-tier results. A +// session that vanishes from reads would look like a successful fix. +// +// Three routes to that class, in precedence order: +// +// 1. The store applies the policy itself (StoragePolicySelfApplying). A plain +// Create through it is already policy-correct. It wins over route 2 on +// purpose: the class named in route 2 is a hardcoded default, while a +// self-applying store resolves the CONFIGURED class and can honor a +// [beads.policies.session] storage override that this package cannot see. +// 2. The store accepts a class out of band (beads.StorageCreateStore). Here the +// class is hardcoded to StorageNoHistory, the policy DEFAULT for sessions; +// a configured override is not visible on this route. That divergence is +// bounded by route 1 taking precedence: gascity's own wiring always hands +// this front door a policy-wrapped store (cmd/gc/class_store.go +// resolveSessionStore over the policy-wrapped city store), so route 2 is +// reached only by a caller that passes a bare store directly — a fixture or +// an embedder — where there is no city config to diverge from. +// 3. Neither. The class is stamped directly onto the bead's own NoHistory +// field — the same field routing the caching store's own fallback relies on +// (internal/beads/caching_store_writes.go) — so backends that route on the +// field still place the bead correctly, and the loss is reported. +// +// Route 3 warns rather than fails. Observability must never break the caller: a +// session that cannot be created is worse than one created in the wrong tier. +// But it must be REPORTED, not silent — an unhonored capability coerced into +// the quiet default is exactly ADR-0043 Cause 1, and it would let this whole +// mechanism ship inert and invisible. func (s *Store) createSessionBead(b beads.Bead) (beads.Bead, error) { + if _, ok := s.store.Store.(StoragePolicySelfApplying); ok { + return s.store.Create(b) + } if storageStore, ok := s.store.Store.(beads.StorageCreateStore); ok { return storageStore.CreateWithStorage(b, beads.StorageNoHistory) } - if _, ok := s.store.Store.(storagePolicySelfApplying); ok { - // The wrapper applies the session storage policy itself; a plain Create - // through it still lands the bead in wisps. Verified on the live fleet - // 2026-08-06: 27 sessions -> wisps, 0 -> issues, all through this path. - return s.store.Create(b) - } warnSessionStorageUnsupported(s.store.Store) + b.NoHistory = true + b.Ephemeral = false return s.store.Create(b) } @@ -135,13 +154,34 @@ var ( sessionStorageWarnTypes = map[string]bool{} ) -// warnSessionStorageUnsupported reports, once per process PER STORE TYPE, that -// the session storage policy could not be applied. Once-per-process was wrong: -// a single benign false alarm burned the guard, and a later genuinely -// incapable chain of a DIFFERENT type then wrote sessions to the committed -// table with no warning at all — a silent failure inside the warning that -// exists to prevent silent failure. Per-type keeps the noise bounded (one line -// per offending type per process) without ever muting a new offender. +// ResetStorageWarningsForTest clears the per-store-type warning ledger. +// +// The ledger is process-wide by design, which makes "this composition does not +// warn" depend on whether an earlier test in the same binary already warned for +// the same store type — a silence assertion that passes for the wrong reason is +// a guard that cannot fail. Cross-package tests (cmd/gc's policy-composition +// guard) reset it first so they observe the FIRST-warning behavior. +func ResetStorageWarningsForTest() { + sessionStorageWarnMu.Lock() + sessionStorageWarnTypes = map[string]bool{} + sessionStorageWarnMu.Unlock() +} + +// warnSessionStorageUnsupported reports that the session storage policy could +// not be requested through a store, ONCE PER STORE TYPE per process. +// +// The key is the store type, not the process, because a single benign warning +// would otherwise mute a later, genuinely incapable chain of a DIFFERENT type — +// a silent failure inside the warning that exists to prevent silent failure. +// Keying on the type bounds the noise (one line per offending type) without +// ever muting a new offender. +// +// The message names the store TYPE and the POLICY that was lost, not a table or +// a commit count: the same code path serves FileStore and MemStore chains where +// there is no Dolt, no issues table, and no commit to count. What is universally +// true is that the policy could not be expressed through this store, so the +// tier now depends on the backend honoring a field rather than on gascity +// having asked for it. func warnSessionStorageUnsupported(store any) { typeName := fmt.Sprintf("%T", store) sessionStorageWarnMu.Lock() @@ -152,10 +192,13 @@ func warnSessionStorageUnsupported(store any) { return } fmt.Fprintf(os.Stderr, - "gc: session storage policy NOT applied: %s does not implement "+ - "beads.StorageCreateStore, so session beads are being written to the "+ - "committed issues table (one DOLT_COMMIT each) instead of wisps. "+ - "This is vp-ia76 / vp-9u1 and it silently inflates the store.\n", + "gc: session storage policy NOT applied through %s: it implements neither "+ + "beads.StorageCreateStore nor session.StoragePolicySelfApplying, so the "+ + "no_history class could not be requested. The bead is stamped no_history "+ + "on its own field and created anyway — backends that route on that field "+ + "still place it in the no-history tier, but a backend that ignores the "+ + "field keeps session beads in its default, fully-retained tier and the "+ + "session storage policy is lost there (vp-ia76 / vp-9u1).\n", typeName) } diff --git a/internal/session/create_storage_test.go b/internal/session/create_storage_test.go index 9e56c3b889..5d9173cef1 100644 --- a/internal/session/create_storage_test.go +++ b/internal/session/create_storage_test.go @@ -10,6 +10,56 @@ import ( "github.com/gastownhall/gascity/internal/beads" ) +// captureStderr redirects os.Stderr for the duration of fn and returns what was +// written to it. +// +// The restore is DEFERRED, not sequential. A t.Fatalf anywhere inside fn runs +// runtime.Goexit, which skips straight past a restore written after the call — +// leaving the whole test binary with os.Stderr pointed at an orphaned pipe, so +// every later test's diagnostics vanish into a buffer nobody reads and the +// failure looks like it came from somewhere else entirely. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + defer r.Close() //nolint:errcheck + + old := os.Stderr + os.Stderr = w + restored := false + restore := func() { + if restored { + return + } + restored = true + os.Stderr = old + w.Close() //nolint:errcheck + } + defer restore() + + fn() + + // Restore before reading: the copy below drains the pipe to EOF, which + // only arrives once the write end is closed. + restore() + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("read captured stderr: %v", err) + } + return buf.String() +} + +// resetSessionStorageWarnTypes clears the per-type warn ledger so a test sees +// the first-warning behavior regardless of what ran before it, and leaves it +// clean for whatever runs after. +func resetSessionStorageWarnTypes(t *testing.T) { + t.Helper() + ResetStorageWarningsForTest() + t.Cleanup(ResetStorageWarningsForTest) +} + // recordingStorageStore implements both Create and CreateWithStorage so a test can // tell WHICH door a create went through. Deliberately not a mock of the function // under test: it is a store, and CreateSessionInfo drives it for real. @@ -31,15 +81,18 @@ func (r *recordingStorageStore) CreateWithStorage(b beads.Bead, storage beads.St return b, nil } -// plainOnlyStore implements Create but NOT CreateWithStorage — the shape of -// NativeDoltStore, which is why the silent-fallback hazard is real. +// plainOnlyStore implements Create and nothing else — no storage class in, no +// policy of its own. It records the bead exactly as it arrives so a test can +// see what the front door managed to carry through the last resort. type plainOnlyStore struct { beads.Store plainCreates int + last beads.Bead } func (p *plainOnlyStore) Create(b beads.Bead) (beads.Bead, error) { p.plainCreates++ + p.last = b return b, nil } @@ -47,10 +100,9 @@ func newSpec() CreateSpec { return CreateSpec{ID: "vc-wisp-test1", Title: "t", AgentName: "a"} } -// A session bead MUST be created under the no_history storage class. Before vp-ia76 -// this front door called Create() directly, so gascity's own session policy was -// dropped and every session landed in the committed issues table with its own -// DOLT_COMMIT — 262/24h, measured. +// A session bead MUST be created under the no_history storage class: a store +// that accepts a class out of band must be asked for one, not called through +// the plain Create that carries no policy at all. func TestCreateSessionInfoAppliesNoHistoryStorage(t *testing.T) { rec := &recordingStorageStore{} s := NewStore(beads.SessionStore{Store: rec}) @@ -60,8 +112,9 @@ func TestCreateSessionInfoAppliesNoHistoryStorage(t *testing.T) { } if rec.storageCreates != 1 { t.Errorf("CreateWithStorage calls = %d, want 1 "+ - "(the session storage policy was dropped; beads land in the committed "+ - "issues table with a DOLT_COMMIT each)", rec.storageCreates) + "(the session storage policy was dropped: the bead is created with no "+ + "class at all and lands in the backend's default, fully-retained tier)", + rec.storageCreates) } if rec.plainCreates != 0 { t.Errorf("plain Create calls = %d, want 0 (policy bypassed)", rec.plainCreates) @@ -73,8 +126,8 @@ func TestCreateSessionInfoAppliesNoHistoryStorage(t *testing.T) { // no_history and ephemeral are NOT interchangeable. ephemeral sets ephemeral=1, which // gascity's own policy declares incompatible for sessions and which matchesTier -// silently DROPS from query results — so using it would make sessions vanish from -// reads while looking like a successful fix. +// silently DROPS from default-tier query results — so using it would make sessions +// vanish from reads while looking like a successful fix. func TestSessionStorageIsNoHistoryAndNotEphemeral(t *testing.T) { rec := &recordingStorageStore{} s := NewStore(beads.SessionStore{Store: rec}) @@ -92,38 +145,21 @@ func TestSessionStorageIsNoHistoryAndNotEphemeral(t *testing.T) { } } -// THE FIX MUST NOT BE ABLE TO SHIP INERT. CachingStore.CreateWithStorage silently -// degrades to Create when its backing store lacks StorageCreateStore, and -// NativeDoltStore lacks it. A chain assembled that way would take this fix, report -// success, and keep writing to issues. The create must still succeed — observability -// must never break the caller — but it must be REPORTED, not silent. -func TestUnsupportedStorageStillCreatesButIsReported(t *testing.T) { +// THE LAST RESORT MUST NOT BE INERT, AND MUST NOT BE SILENT. When a store offers +// neither route to a storage class, the front door stamps the class onto the +// bead's own field (the routing every backend performs) AND says so. Asserting +// only that the create succeeded would make a test named "...IsReported" prove +// nothing about reporting. +func TestUnsupportedStorageStampsClassAndIsReported(t *testing.T) { + resetSessionStorageWarnTypes(t) + plain := &plainOnlyStore{} s := NewStore(beads.SessionStore{Store: plain}) - // Capture stderr for real. Asserting only that the create SUCCEEDED would make - // this test named "...IsReported" while proving nothing about reporting — a guard - // that cannot fail, which is the exact pattern this codebase keeps producing. - old := os.Stderr - r, w, err := os.Pipe() - if err != nil { - t.Fatalf("pipe: %v", err) - } - os.Stderr = w - sessionStorageWarnMu.Lock() - sessionStorageWarnTypes = map[string]bool{} - sessionStorageWarnMu.Unlock() - - _, createErr := s.CreateSessionInfo(newSpec()) - - if err := w.Close(); err != nil { - t.Fatalf("close stderr pipe: %v", err) - } - os.Stderr = old - var buf bytes.Buffer - if _, err := io.Copy(&buf, r); err != nil { - t.Fatalf("read captured stderr: %v", err) - } + var createErr error + out := captureStderr(t, func() { + _, createErr = s.CreateSessionInfo(newSpec()) + }) if createErr != nil { t.Fatalf("CreateSessionInfo must not fail when storage is unsupported: %v", createErr) @@ -132,15 +168,42 @@ func TestUnsupportedStorageStillCreatesButIsReported(t *testing.T) { t.Errorf("plain Create calls = %d, want 1 (the bead must still be persisted)", plain.plainCreates) } - if !strings.Contains(buf.String(), "session storage policy NOT applied") { - t.Errorf("no warning on stderr when the storage class could not be applied; "+ - "the fix would ship INERT and INVISIBLE — sessions keep landing in the "+ - "committed issues table while the change reports success. got: %q", - buf.String()) + if !plain.last.NoHistory { + t.Errorf("bead reached the store with NoHistory = false: warning without stamping " + + "drops the policy on the floor even though the field routing that would have " + + "carried it costs nothing") } - if !strings.Contains(buf.String(), "plainOnlyStore") { + if plain.last.Ephemeral { + t.Errorf("bead reached the store with Ephemeral = true: ephemeral beads are " + + "GC/TTL-eligible and are dropped from default-tier reads") + } + if !strings.Contains(out, "session storage policy NOT applied") { + t.Errorf("no warning on stderr when the storage class could not be requested; "+ + "an operator has no signal that the chain is assembled wrong. got: %q", out) + } + if !strings.Contains(out, "plainOnlyStore") { t.Errorf("warning does not name the offending store type, so an operator "+ - "cannot tell which chain dropped the policy. got: %q", buf.String()) + "cannot tell which chain dropped the policy. got: %q", out) + } +} + +// The warning describes the POLICY that was lost, not a Dolt table or a commit +// count: the same path serves FileStore and MemStore chains where there is no +// issues table and nothing to commit, and a warning that asserts otherwise +// sends an operator hunting for a table that does not exist. +func TestWarningDoesNotAssertDoltSpecificConsequences(t *testing.T) { + resetSessionStorageWarnTypes(t) + + out := captureStderr(t, func() { warnSessionStorageUnsupported(&plainOnlyStore{}) }) + + for _, forbidden := range []string{"issues table", "DOLT_COMMIT", "Dolt commit"} { + if strings.Contains(out, forbidden) { + t.Errorf("warning asserts %q, which is false for non-Dolt backends: %q", + forbidden, out) + } + } + if !strings.Contains(out, "no_history") { + t.Errorf("warning does not name the storage class that was lost: %q", out) } } @@ -157,34 +220,14 @@ func (p *policySelfApplyingStore) Create(b beads.Bead) (beads.Bead, error) { } func (p *policySelfApplyingStore) AppliesBeadStoragePolicy() {} -// A policy-self-applying wrapper must create WITHOUT any warning: warning here -// was the false alarm that burned the old process-wide once-guard. +// A policy-self-applying store must create WITHOUT any warning: warning here +// would be a false alarm about a policy that was in fact applied. func TestPolicySelfApplyingStoreCreatesQuietly(t *testing.T) { - captureStderr := func(fn func()) string { - old := os.Stderr - r, w, err := os.Pipe() - if err != nil { - t.Fatalf("pipe: %v", err) - } - os.Stderr = w - fn() - if err := w.Close(); err != nil { - t.Fatalf("close: %v", err) - } - os.Stderr = old - var buf bytes.Buffer - if _, err := io.Copy(&buf, r); err != nil { - t.Fatalf("copy: %v", err) - } - return buf.String() - } - sessionStorageWarnMu.Lock() - sessionStorageWarnTypes = map[string]bool{} - sessionStorageWarnMu.Unlock() + resetSessionStorageWarnTypes(t) pol := &policySelfApplyingStore{} s := NewStore(beads.SessionStore{Store: pol}) - out := captureStderr(func() { + out := captureStderr(t, func() { if _, err := s.CreateSessionInfo(newSpec()); err != nil { t.Fatalf("CreateSessionInfo: %v", err) } @@ -197,31 +240,65 @@ func TestPolicySelfApplyingStoreCreatesQuietly(t *testing.T) { } } +// storageAndPolicyStore offers BOTH routes: an out-of-band storage class and its +// own policy application. The self-applying store must win — it is the only one +// that can see a CONFIGURED session storage class, while the class the front +// door would pass is a hardcoded default. +type storageAndPolicyStore struct { + beads.Store + plainCreates int + storageCreates int +} + +func (p *storageAndPolicyStore) Create(b beads.Bead) (beads.Bead, error) { + p.plainCreates++ + return b, nil +} + +func (p *storageAndPolicyStore) CreateWithStorage(b beads.Bead, _ beads.StorageClass) (beads.Bead, error) { + p.storageCreates++ + return b, nil +} +func (p *storageAndPolicyStore) AppliesBeadStoragePolicy() {} + +func TestSelfAppliedPolicyWinsOverHardcodedClass(t *testing.T) { + resetSessionStorageWarnTypes(t) + + both := &storageAndPolicyStore{} + s := NewStore(beads.SessionStore{Store: both}) + if _, err := s.CreateSessionInfo(newSpec()); err != nil { + t.Fatalf("CreateSessionInfo: %v", err) + } + if both.storageCreates != 0 { + t.Errorf("CreateWithStorage calls = %d, want 0: the front door imposed its "+ + "hardcoded no_history class on a store that resolves the CONFIGURED class "+ + "itself, silently overriding a [beads.policies.session] storage override", + both.storageCreates) + } + if both.plainCreates != 1 { + t.Errorf("plain Create calls = %d, want 1", both.plainCreates) + } +} + // One benign incapable type must not mute the warning for a DIFFERENT // incapable type later in the same process (the burned-once-guard defect). func TestWarnIsPerStoreTypeNotPerProcess(t *testing.T) { - sessionStorageWarnMu.Lock() - sessionStorageWarnTypes = map[string]bool{} - sessionStorageWarnMu.Unlock() + resetSessionStorageWarnTypes(t) - warnSessionStorageUnsupported(&plainOnlyStore{}) - old := os.Stderr - r, w, err := os.Pipe() - if err != nil { - t.Fatalf("pipe: %v", err) - } - os.Stderr = w - warnSessionStorageUnsupported(&policySelfApplyingStore{}) // different type: must warn - warnSessionStorageUnsupported(&plainOnlyStore{}) // repeat type: must stay quiet - if err := w.Close(); err != nil { - t.Fatalf("close: %v", err) - } - os.Stderr = old - var buf bytes.Buffer - if _, err := io.Copy(&buf, r); err != nil { - t.Fatalf("copy: %v", err) + // Capture from the FIRST call: it warns for real, and letting it reach the + // real stderr both pollutes the test output and hides a regression where + // the first call is the one that misbehaves. + out := captureStderr(t, func() { + warnSessionStorageUnsupported(&plainOnlyStore{}) // first of its type: warns + warnSessionStorageUnsupported(&policySelfApplyingStore{}) // different type: must warn + warnSessionStorageUnsupported(&plainOnlyStore{}) // repeat type: must stay quiet + }) + + if n := strings.Count(out, "NOT applied"); n != 2 { + t.Errorf("want exactly 2 warnings (one per distinct type, the repeat muted), got %d: %q", + n, out) } - if n := strings.Count(buf.String(), "NOT applied"); n != 1 { - t.Errorf("want exactly 1 warning for the new type (repeat muted), got %d: %q", n, buf.String()) + if !strings.Contains(out, "plainOnlyStore") || !strings.Contains(out, "policySelfApplyingStore") { + t.Errorf("both offending types must be named; got %q", out) } }