diff --git a/cmd/gc/bead_policy_store.go b/cmd/gc/bead_policy_store.go index 067aa97067..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 @@ -50,6 +59,15 @@ 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). +// 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 ( _ beads.BatchDeleter = (*beadPolicyStore)(nil) _ beads.BatchDeleter = (*beadPolicyGraphStore)(nil) 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..a831daa54b --- /dev/null +++ b/cmd/gc/session_storage_policy_wiring_test.go @@ -0,0 +1,205 @@ +package main + +import ( + "bytes" + "context" + "io" + "os" + "strings" + "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 — 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 +// 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 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 +// 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. +// +// 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") + } +} + +// 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/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..6d5982748c 100644 --- a/internal/beads/caching_store_writes.go +++ b/internal/beads/caching_store_writes.go @@ -15,14 +15,52 @@ 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). +// +// 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) { - 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..c7982c0904 --- /dev/null +++ b/internal/beads/native_dolt_storage_class_live_test.go @@ -0,0 +1,203 @@ +//go:build integration + +package beads + +import ( + "context" + "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 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 -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) { + 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") + } + 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 { + 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..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) ) @@ -821,6 +822,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) 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 6b2ca373ba..0c8a02531d 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,145 @@ 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 } +// 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. +// +// 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. +// +// 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) + } + warnSessionStorageUnsupported(s.store.Store) + b.NoHistory = true + b.Ephemeral = false + return s.store.Create(b) +} + +var ( + sessionStorageWarnMu sync.Mutex + sessionStorageWarnTypes = map[string]bool{} +) + +// 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() + seen := sessionStorageWarnTypes[typeName] + sessionStorageWarnTypes[typeName] = true + sessionStorageWarnMu.Unlock() + if seen { + return + } + fmt.Fprintf(os.Stderr, + "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) +} + // 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..5d9173cef1 --- /dev/null +++ b/internal/session/create_storage_test.go @@ -0,0 +1,304 @@ +package session + +import ( + "bytes" + "io" + "os" + "strings" + "testing" + + "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. +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 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 +} + +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: 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}) + + 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: 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) + } + 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 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}) + + 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 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}) + + 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) + } + if plain.plainCreates != 1 { + t.Errorf("plain Create calls = %d, want 1 (the bead must still be persisted)", + plain.plainCreates) + } + 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 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", 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) + } +} + +// 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 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) { + resetSessionStorageWarnTypes(t) + + pol := &policySelfApplyingStore{} + s := NewStore(beads.SessionStore{Store: pol}) + out := captureStderr(t, 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) + } +} + +// 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) { + resetSessionStorageWarnTypes(t) + + // 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 !strings.Contains(out, "plainOnlyStore") || !strings.Contains(out, "policySelfApplyingStore") { + t.Errorf("both offending types must be named; got %q", out) + } +}