From 47e23feeaf7eeb28ae16a4b3c8a4f6b6be3bca52 Mon Sep 17 00:00:00 2001 From: Quan Huynh Date: Wed, 8 Jul 2026 12:09:51 +0700 Subject: [PATCH 1/2] feat: support redis cluster mode --- cmd/main.go | 30 +++- pkg/agent/brain_log_test.go | 36 +++++ pkg/agent/cursors.go | 26 +++- pkg/config/config.go | 28 ++++ pkg/config/redis_test.go | 60 ++++++++ pkg/core/oncall.go | 6 +- pkg/storage/postgres.go | 199 +++++++++++++++++++++++++- pkg/storage/postgres_connect_test.go | 107 ++++++++++++++ pkg/storage/postgres_hint_test.go | 96 +++++++++++++ src/_sidebar.md | 1 + src/agent/data-sources.md | 8 ++ src/configuration/configuration.md | 37 ++++- src/configuration/postgres-storage.md | 43 ++++++ ui/src/pages/PatternsPage.test.tsx | 47 ++++++ ui/src/pages/PatternsPage.tsx | 15 ++ 15 files changed, 718 insertions(+), 21 deletions(-) create mode 100644 pkg/storage/postgres_connect_test.go create mode 100644 pkg/storage/postgres_hint_test.go create mode 100644 src/configuration/postgres-storage.md diff --git a/cmd/main.go b/cmd/main.go index c6e8307..eb2125d 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -145,13 +145,11 @@ func main() { // Shared Redis client used by both on-call and the agent worker. We open // it once here so both subsystems share connections. - var sharedRedis *redis.Client + var sharedRedis redis.UniversalClient if cfg.OnCall.Enable || cfg.OnCall.InitializedOnly { - redisOptions := handlerRedisOptions(cfg.Redis) - - // Initialize Redis client - redisClient := redis.NewClient(redisOptions) + // Initialize Redis client (cluster-aware when redis.cluster is set) + redisClient := newRedisClient(cfg.Redis) // Test Redis connection if err := redisClient.Ping(context.Background()).Err(); err != nil { @@ -179,7 +177,7 @@ func main() { // in-memory cursors when Redis isn't reachable). rdb := sharedRedis if rdb == nil && cfg.Redis.Host != "" { - rdb = redis.NewClient(handlerRedisOptions(cfg.Redis)) + rdb = newRedisClient(cfg.Redis) if err := rdb.Ping(context.Background()).Err(); err != nil { log.Printf("agent: Redis unavailable (%v); cursors will be in-memory only", err) rdb = nil @@ -225,7 +223,7 @@ func main() { // startAgent constructs the worker, starts it in a goroutine, and registers // admin routes on the fiber app. It returns the catalog so the caller can // hold a reference (and so future hot-reload code has a handle to it). -func startAgent(ctx context.Context, app *fiber.App, cfg c.AgentConfig, gatewaySecret string, store storage.Provider, rdb *redis.Client) (*agent.Catalog, error) { +func startAgent(ctx context.Context, app *fiber.App, cfg c.AgentConfig, gatewaySecret string, store storage.Provider, rdb redis.UniversalClient) (*agent.Catalog, error) { // On the Postgres backend, install the typed signal-table // catalog store so the log catalog reads/writes the explicit // vs_patterns/vs_logs/vs_services tables (searchable, indexed) instead of @@ -408,6 +406,24 @@ func handleQueueMessage(content *map[string]interface{}) error { return services.CreateIncident("", content, &overwrite) // teamID as empty string } +// newRedisClient builds the Redis client both subsystems share. It reuses +// handlerRedisOptions for the addr/password/TLS settings, then returns either a +// single-node client (the default) or a cluster-aware client when the operator +// opts in via redis.cluster / REDIS_CLUSTER. Both concrete types satisfy +// redis.UniversalClient, so callers thread the interface and single-key +// Get/Set follow MOVED/ASK redirects transparently in cluster mode. +func newRedisClient(rc c.RedisConfig) redis.UniversalClient { + opts := handlerRedisOptions(rc) + if rc.ClusterEnabled() { + return redis.NewClusterClient(&redis.ClusterOptions{ + Addrs: []string{opts.Addr}, + Password: opts.Password, + TLSConfig: opts.TLSConfig, + }) + } + return redis.NewClient(opts) +} + func handlerRedisOptions(rc c.RedisConfig) *redis.Options { redisOptions := &redis.Options{ Addr: rc.Host + ":" + strconv.Itoa(rc.Port), diff --git a/pkg/agent/brain_log_test.go b/pkg/agent/brain_log_test.go index 7ef1b9f..26d5064 100644 --- a/pkg/agent/brain_log_test.go +++ b/pkg/agent/brain_log_test.go @@ -379,3 +379,39 @@ func TestLogBrain_AutoPromoteAfter_AlreadyKnownStaysKnown(t *testing.T) { t.Fatalf("catalog verdict = %q, want known (an already-known pattern must stay known)", c.Get("p").Verdict) } } + +// (f) The operator's real "first fetch" scenario: an Elasticsearch source's +// FIRST batch lands a pattern whose sighting count is already far above the +// threshold (e.g. 3304 >= 100) in a single tick. It must classify known +// immediately — Promote flips the stored verdict and LogReadiness reports Ready +// — not sit "still learning" forever. This pins the count clause of isLogKnown +// on the first-batch path so a large first pull is never stuck learning once the +// threshold is the (non-zero) default. +func TestLogBrain_AutoPromoteAfter_FirstBatchAboveThresholdIsKnown(t *testing.T) { + cat := config.AgentCatalogConfig{ + AutoPromoteAfter: 100, // the embedded default an omitted key resolves to + SpikeMultiplier: 0, // isolate the count-promotion path from spike + } + b, c := newLogBrainForTest(t, cat) + + v := classifyOnce(t, b, logObs("p", 3304)) // single first-fetch batch + if got := c.Get("p").Count; got != 3304 { + t.Fatalf("count = %d, want 3304", got) + } + if v.Class != core.VerdictKnownPattern { + t.Fatalf("first-batch verdict = %v, want known (3304 >= 100)", v.Class) + } + if c.Get("p").Verdict != "known" { + t.Fatalf("catalog verdict = %q, want known (Promote must fire on the first batch)", c.Get("p").Verdict) + } + // The read-side readiness the UI consumes must agree with the classifier: + // Ready, with the threshold as the target — never the Needed=0 "no target" + // state for a pattern already past the (non-zero) threshold. + r := LogReadiness(c.Get("p"), cat.AutoPromoteAfter, 30*time.Second) + if !r.Ready { + t.Fatalf("LogReadiness.Ready = false, want true (3304 >= 100)") + } + if r.Needed != 100 { + t.Fatalf("LogReadiness.Needed = %d, want 100 (threshold target, not the no-target sentinel)", r.Needed) + } +} diff --git a/pkg/agent/cursors.go b/pkg/agent/cursors.go index 2b1141c..8d2c7b2 100644 --- a/pkg/agent/cursors.go +++ b/pkg/agent/cursors.go @@ -13,7 +13,7 @@ import ( // falls back to in-memory storage when Redis is unavailable so that // development setups don't require Redis just to try training mode. type CursorStore struct { - rdb *redis.Client + rdb redis.UniversalClient keyPrefix string mu sync.RWMutex @@ -21,7 +21,7 @@ type CursorStore struct { } // NewCursorStore returns a CursorStore. Pass `rdb=nil` for in-memory only. -func NewCursorStore(rdb *redis.Client) *CursorStore { +func NewCursorStore(rdb redis.UniversalClient) *CursorStore { return &CursorStore{ rdb: rdb, keyPrefix: "versus:agent:cursor:", @@ -77,14 +77,30 @@ func (s *CursorStore) Reset(ctx context.Context) error { if s.rdb == nil { return nil } + // In cluster mode a plain SCAN only walks the node the call routes to, + // so sweep every master shard; in single-node mode there is exactly one + // backend and the single deleteCursorKeys pass covers it. + if cc, ok := s.rdb.(*redis.ClusterClient); ok { + return cc.ForEachMaster(ctx, func(ctx context.Context, m *redis.Client) error { + return s.deleteCursorKeys(ctx, m) + }) + } + return s.deleteCursorKeys(ctx, s.rdb) +} + +// deleteCursorKeys scans one Redis backend for every cursor key under the +// prefix and deletes them ONE AT A TIME. Single-key Del always routes to the +// key's owning slot, so it is correct on a *redis.ClusterClient too — a +// multi-key Del across different hash slots would fail with CROSSSLOT. +func (s *CursorStore) deleteCursorKeys(ctx context.Context, c redis.Cmdable) error { var cursor uint64 for { - keys, next, err := s.rdb.Scan(ctx, cursor, s.keyPrefix+"*", 100).Result() + keys, next, err := c.Scan(ctx, cursor, s.keyPrefix+"*", 100).Result() if err != nil { return err } - if len(keys) > 0 { - if err := s.rdb.Del(ctx, keys...).Err(); err != nil { + for _, k := range keys { + if err := c.Del(ctx, k).Err(); err != nil { return err } } diff --git a/pkg/config/config.go b/pkg/config/config.go index fa5f892..5cf0a63 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -236,6 +236,16 @@ type RedisConfig struct { DB int `mapstructure:"db"` TLS *bool `mapstructure:"tls"` InsecureSkipVerify bool `mapstructure:"insecure_skip_verify"` + Cluster *bool `mapstructure:"cluster"` +} + +// ClusterEnabled reports whether the Redis client should be built in +// cluster mode (redis.NewClusterClient) so it follows MOVED/ASK redirects +// across shards — required for AWS ElastiCache Valkey/Redis in cluster mode. +// Single-node is the default: a nil flag (key omitted) preserves today's +// behaviour. Set redis.cluster: true (or REDIS_CLUSTER=true) to opt in. +func (r RedisConfig) ClusterEnabled() bool { + return r.Cluster != nil && *r.Cluster } // TLSEnabled reports whether the Redis client should dial over TLS. TLS is @@ -256,6 +266,16 @@ func redisTLSFromEnv(v string) *bool { return &enabled } +// redisClusterFromEnv resolves a non-empty REDIS_CLUSTER value to a cluster +// flag, fail-safe OFF: only an explicit on-value (true/1/yes/on, any case) +// enables cluster mode; every other value (a typo, empty, off, ...) keeps the +// single-node default so an operator never accidentally switches topology. +func redisClusterFromEnv(v string) *bool { + on := map[string]bool{"true": true, "1": true, "yes": true, "on": true} + enabled := on[strings.ToLower(strings.TrimSpace(v))] + return &enabled +} + var ( cfg *Config cfgOnce sync.Once @@ -352,6 +372,14 @@ func loadConfigFromPath(path string) (*Config, error) { loaded.Redis.TLS = redisTLSFromEnv(val) } + // Redis env override: REDIS_CLUSTER toggles cluster-mode client + // construction. Fail-safe OFF: only an explicit on-value (true/1/yes/on, + // any case) enables cluster mode; unset or any other value keeps the + // single-node default so topology is never switched by accident. + if val := os.Getenv("REDIS_CLUSTER"); val != "" { + loaded.Redis.Cluster = redisClusterFromEnv(val) + } + // Storage env overrides if t := os.Getenv("STORAGE_TYPE"); t != "" { loaded.Storage.Type = t diff --git a/pkg/config/redis_test.go b/pkg/config/redis_test.go index 60a880d..6c0f584 100644 --- a/pkg/config/redis_test.go +++ b/pkg/config/redis_test.go @@ -62,3 +62,63 @@ func TestRedisTLSFromEnv(t *testing.T) { }) } } + +// TestRedisConfigClusterEnabled covers the cluster opt-in: single-node is the +// default when the flag is omitted, an explicit true enables cluster mode. +func TestRedisConfigClusterEnabled(t *testing.T) { + tru := true + fls := false + + tests := []struct { + name string + cluster *bool + want bool + }{ + {"omitted defaults to single-node", nil, false}, + {"explicit true", &tru, true}, + {"explicit false", &fls, false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + rc := RedisConfig{Cluster: tc.cluster} + if got := rc.ClusterEnabled(); got != tc.want { + t.Fatalf("ClusterEnabled() = %v, want %v", got, tc.want) + } + }) + } +} + +// TestRedisClusterFromEnv covers the fail-safe-OFF parse: only an explicit +// on-value (true/1/yes/on, any case) enables cluster mode; every other value +// — an off-value, a typo, or garbage — keeps the single-node default. Unset +// is handled by the caller (no override) and stays off via ClusterEnabled(). +func TestRedisClusterFromEnv(t *testing.T) { + tests := []struct { + value string + want bool // resolved ClusterEnabled() + }{ + {"true", true}, + {"TRUE", true}, + {"1", true}, + {"yes", true}, + {"on", true}, + {" true ", true}, + {"false", false}, + {"0", false}, + {"no", false}, + {"off", false}, + {"ture", false}, // typo stays single-node + {"garbage", false}, + {"", false}, + } + + for _, tc := range tests { + t.Run(tc.value, func(t *testing.T) { + got := RedisConfig{Cluster: redisClusterFromEnv(tc.value)}.ClusterEnabled() + if got != tc.want { + t.Fatalf("redisClusterFromEnv(%q) → ClusterEnabled() = %v, want %v", tc.value, got, tc.want) + } + }) + } +} diff --git a/pkg/core/oncall.go b/pkg/core/oncall.go index e08d08b..9fd58f7 100644 --- a/pkg/core/oncall.go +++ b/pkg/core/oncall.go @@ -23,7 +23,7 @@ var CreateOnCallProvider func(cfg *config.Config, awsClient *ssmincidents.Client // OnCallWorkflow coordinates on-call escalation with a single provider type OnCallWorkflow struct { provider OnCallProvider - redisClient *redis.Client + redisClient redis.UniversalClient } // Global instance for singleton access @@ -33,7 +33,7 @@ var ( ) // NewOnCallWorkflow creates a new on-call workflow with the given provider -func NewOnCallWorkflow(redisClient *redis.Client, provider OnCallProvider) *OnCallWorkflow { +func NewOnCallWorkflow(redisClient redis.UniversalClient, provider OnCallProvider) *OnCallWorkflow { return &OnCallWorkflow{ provider: provider, redisClient: redisClient, @@ -42,7 +42,7 @@ func NewOnCallWorkflow(redisClient *redis.Client, provider OnCallProvider) *OnCa // InitOnCallWorkflow initializes the global singleton instance // This is called once from main.go with the Redis client and AWS client -func InitOnCallWorkflow(awsClient *ssmincidents.Client, redisClient *redis.Client) { +func InitOnCallWorkflow(awsClient *ssmincidents.Client, redisClient redis.UniversalClient) { once.Do(func() { cfg := config.GetConfig() diff --git a/pkg/storage/postgres.go b/pkg/storage/postgres.go index 7b7dcb4..85efe5f 100644 --- a/pkg/storage/postgres.go +++ b/pkg/storage/postgres.go @@ -7,10 +7,14 @@ import ( "encoding/json" "errors" "fmt" + "log" + "net/url" + "os" "strings" "time" // pgx stdlib driver — registers "pgx" with database/sql. + "github.com/jackc/pgx/v5/pgconn" _ "github.com/jackc/pgx/v5/stdlib" ) @@ -34,8 +38,164 @@ type postgresProvider struct { // pool. The pool is owned by this provider — callers MUST NOT Close it. func (p *postgresProvider) DB() *sql.DB { return p.db } +// defaultConnectBudget bounds the total time NewPostgres will spend retrying +// the initial Postgres reachability check before giving up. It is overridable +// per deployment via the POSTGRES_CONNECT_TIMEOUT env var. +const defaultConnectBudget = 60 * time.Second + +// perPingTimeout bounds a single reachability probe so a black-holed host +// (dropped packets, wrong host, firewall) can never block on the OS TCP +// connect timeout — the probe fails fast and the retry loop logs and backs off. +const perPingTimeout = 5 * time.Second + +// connectBudget resolves the total connect budget from POSTGRES_CONNECT_TIMEOUT +// (a Go duration string such as "90s"), falling back to defaultConnectBudget +// when the var is unset, unparseable, or non-positive. +func connectBudget() time.Duration { + raw := strings.TrimSpace(os.Getenv("POSTGRES_CONNECT_TIMEOUT")) + if raw == "" { + return defaultConnectBudget + } + d, err := time.ParseDuration(raw) + if err != nil || d <= 0 { + return defaultConnectBudget + } + return d +} + +// redactDSN returns a safe "host:port/dbname" summary of a Postgres DSN for +// logs and errors. It NEVER returns the password: URL-form userinfo is stripped +// of its password and keyword-form drops the password= token entirely. If the +// DSN cannot be understood it returns the constant "postgres" rather than +// risk leaking any raw connection string. +func redactDSN(dsn string) string { + dsn = strings.TrimSpace(dsn) + if dsn == "" { + return "postgres" + } + // URL form: postgres://user:pass@host:5432/db?... + if strings.Contains(dsn, "://") { + u, err := url.Parse(dsn) + if err != nil || u.Host == "" { + return "postgres" + } + hostPort := u.Host + db := strings.TrimPrefix(u.Path, "/") + summary := hostPort + if db != "" { + summary = hostPort + "/" + db + } + if u.User != nil { + if name := u.User.Username(); name != "" { + summary = name + "@" + summary + } + } + return summary + } + // Keyword form: host=... port=... dbname=... password=... + var host, port, dbname string + for _, field := range strings.Fields(dsn) { + kv := strings.SplitN(field, "=", 2) + if len(kv) != 2 { + continue + } + switch strings.ToLower(kv[0]) { + case "host": + host = kv[1] + case "port": + port = kv[1] + case "dbname": + dbname = kv[1] + } + } + if host == "" { + return "postgres" + } + summary := host + if port != "" { + summary = host + ":" + port + } + if dbname != "" { + summary = summary + "/" + dbname + } + return summary +} + +// dsnDBName extracts just the database name from a Postgres DSN (URL form +// postgres://…/dbname or keyword form dbname=…), returning "" when it cannot +// be determined. It shares the URL/keyword shapes with redactDSN but yields +// only the dbname so pgSetupHint can name the real database in its guide +// without ever touching the password. +func dsnDBName(dsn string) string { + dsn = strings.TrimSpace(dsn) + if dsn == "" { + return "" + } + if strings.Contains(dsn, "://") { + u, err := url.Parse(dsn) + if err != nil { + return "" + } + return strings.TrimPrefix(u.Path, "/") + } + for _, field := range strings.Fields(dsn) { + kv := strings.SplitN(field, "=", 2) + if len(kv) == 2 && strings.ToLower(kv[0]) == "dbname" { + return kv[1] + } + } + return "" +} + +// pgSetupHint returns an actionable, operator-facing provisioning guide when +// err is a Postgres login/permission failure, or "" for anything else. It +// inspects for the SQLSTATEs an operator hits when the role can't log in +// (28P01/28000), the database is missing (3D000), or the role lacks the +// GRANTs needed to migrate (42501/3F000 — the PostgreSQL 15+ "permission +// denied for schema public" case). The guide is STATIC text plus dbName only, +// so it can never leak a password or the DSN. dbName falls back to +// "versus_enterprise" when the connection string didn't name a database. +func pgSetupHint(err error, dbName string) string { + if err == nil { + return "" + } + var pgErr *pgconn.PgError + if !errors.As(err, &pgErr) { + return "" + } + switch pgErr.Code { + case "28P01", "28000", // auth failed: invalid_password / invalid_authorization + "3D000", // database does not exist: invalid_catalog_name + "42501", "3F000": // missing GRANTs: insufficient_privilege / invalid_schema_name + default: + return "" + } + if strings.TrimSpace(dbName) == "" { + dbName = "versus_enterprise" + } + return fmt.Sprintf(`postgres rejected the connection: the role/database may be missing or lack privileges. +Provision it once as a superuser (psql), substituting your own db / user / password: + + CREATE DATABASE %[1]s; + CREATE USER versus WITH PASSWORD 'your_strong_password'; + GRANT ALL PRIVILEGES ON DATABASE %[1]s TO versus; + \c %[1]s + GRANT ALL ON SCHEMA public TO versus; -- required on PostgreSQL 15+ + +On PostgreSQL 15+ the public schema no longer allows CREATE by default, so the +final grant (run while connected to %[1]s) is the usual fix for +"permission denied for schema public" during migration.`, dbName) +} + // NewPostgres opens a connection to Postgres, runs idempotent migrations, // and returns a ready Provider. Callers must call Close when done. +// +// The initial reachability check is bounded, retried, and logged so an +// unreachable or slow-to-start database can never turn into a silent restart +// loop: each probe is capped at perPingTimeout, the loop retries with backoff +// until the connectBudget is exhausted, and every attempt logs to stderr +// (captured by `docker logs`) using a redacted host:port/dbname target that +// never contains the password. func NewPostgres(opts PostgresOptions) (Provider, error) { if opts.DSN == "" { return nil, fmt.Errorf("storage: postgres DSN is required (set POSTGRES_DSN)") @@ -44,12 +204,45 @@ func NewPostgres(opts PostgresOptions) (Provider, error) { if err != nil { return nil, fmt.Errorf("storage: open postgres: %w", err) } - if err := db.Ping(); err != nil { - _ = db.Close() - return nil, fmt.Errorf("storage: ping postgres: %w", err) + + redacted := redactDSN(opts.DSN) + dbName := dsnDBName(opts.DSN) + budget := connectBudget() + deadline := time.Now().Add(budget) + + log.Printf("storage: connecting to postgres at %s", redacted) + + var lastErr error + for attempt := 1; ; attempt++ { + ctx, cancel := context.WithTimeout(context.Background(), perPingTimeout) + lastErr = db.PingContext(ctx) + cancel() + if lastErr == nil { + log.Printf("storage: connected to postgres at %s", redacted) + break + } + // Backoff grows linearly from ~1s and is capped at ~5s, but never + // past the remaining budget. + backoff := time.Duration(attempt) * time.Second + if backoff > 5*time.Second { + backoff = 5 * time.Second + } + if time.Now().Add(backoff).After(deadline) { + if hint := pgSetupHint(lastErr, dbName); hint != "" { + log.Printf("%s", hint) + } + _ = db.Close() + return nil, fmt.Errorf("storage: cannot reach postgres at %s after %s: %w", redacted, budget, lastErr) + } + log.Printf("storage: cannot reach postgres at %s (attempt %d): %v; retrying in %s", redacted, attempt, lastErr, backoff) + time.Sleep(backoff) } + p := &postgresProvider{db: db} if err := p.migrate(); err != nil { + if hint := pgSetupHint(err, dbName); hint != "" { + log.Printf("%s", hint) + } _ = db.Close() return nil, fmt.Errorf("storage: postgres migrate: %w", err) } diff --git a/pkg/storage/postgres_connect_test.go b/pkg/storage/postgres_connect_test.go new file mode 100644 index 0000000..5d83d5e --- /dev/null +++ b/pkg/storage/postgres_connect_test.go @@ -0,0 +1,107 @@ +package storage + +// postgres_connect_test.go — covers the bounded, redacted, retried Postgres +// connect hardening: redactDSN must never leak a password, and an unreachable +// DSN must fail within its configured budget instead of hanging. + +import ( + "strings" + "testing" + "time" +) + +func TestRedactDSN(t *testing.T) { + cases := []struct { + name string + dsn string + want string + absent string // substring that must NOT appear (e.g. the password) + }{ + { + name: "url form with password", + dsn: "postgres://versus:s3cr3t@db.internal:5432/incidents?sslmode=disable", + want: "versus@db.internal:5432/incidents", + absent: "s3cr3t", + }, + { + name: "url form without user", + dsn: "postgres://db.internal:5432/incidents", + want: "db.internal:5432/incidents", + absent: "@", + }, + { + name: "keyword form with password", + dsn: "host=db.internal port=5432 dbname=incidents user=versus password=s3cr3t sslmode=disable", + want: "db.internal:5432/incidents", + absent: "s3cr3t", + }, + { + name: "malformed dsn falls back to constant", + dsn: "::::not a dsn::::password=leakme", + want: "postgres", + absent: "leakme", + }, + { + name: "empty dsn falls back to constant", + dsn: "", + want: "postgres", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := redactDSN(tc.dsn) + if got != tc.want { + t.Fatalf("redactDSN(%q) = %q, want %q", tc.dsn, got, tc.want) + } + if tc.absent != "" && strings.Contains(got, tc.absent) { + t.Fatalf("redactDSN(%q) = %q leaked %q", tc.dsn, got, tc.absent) + } + }) + } +} + +// TestRedactDSN_NeverLeaksPassword sweeps a range of DSN shapes to assert the +// literal password token never survives redaction. +func TestRedactDSN_NeverLeaksPassword(t *testing.T) { + const password = "TOPSECRET_PW" + dsns := []string{ + "postgres://user:" + password + "@host:5432/db?sslmode=disable", + "postgresql://user:" + password + "@host/db", + "host=host port=5432 dbname=db user=user password=" + password, + "password=" + password + " host=host dbname=db", + } + for _, dsn := range dsns { + if got := redactDSN(dsn); strings.Contains(got, password) { + t.Fatalf("redactDSN(%q) leaked password: %q", dsn, got) + } + } +} + +// TestNewPostgres_UnreachableIsBounded proves the connect no longer hangs: a +// DSN pointing at a closed port with a tiny budget returns an error quickly +// instead of blocking on the OS TCP connect timeout. +func TestNewPostgres_UnreachableIsBounded(t *testing.T) { + t.Setenv("POSTGRES_CONNECT_TIMEOUT", "2s") + + const dsn = "postgres://versus:versus@127.0.0.1:1/versus?sslmode=disable" + + start := time.Now() + done := make(chan error, 1) + go func() { + _, err := NewPostgres(PostgresOptions{DSN: dsn}) + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("NewPostgres to a closed port should return an error") + } + if strings.Contains(err.Error(), "versus:versus") { + t.Fatalf("error leaked DSN credentials: %v", err) + } + case <-time.After(15 * time.Second): + t.Fatalf("NewPostgres hung on unreachable host (>%s); bound not enforced", time.Since(start)) + } +} diff --git a/pkg/storage/postgres_hint_test.go b/pkg/storage/postgres_hint_test.go new file mode 100644 index 0000000..9745ab4 --- /dev/null +++ b/pkg/storage/postgres_hint_test.go @@ -0,0 +1,96 @@ +package storage + +import ( + "errors" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgconn" +) + +// TestPGSetupHint_ReturnsGuide asserts the provisioning guide is emitted for +// each login/permission SQLSTATE and names the real database, and that the +// PG15+ schema grant line is always present so the operator sees the fix for +// "permission denied for schema public". +func TestPGSetupHint_ReturnsGuide(t *testing.T) { + codes := []struct { + code string + name string + }{ + {"42501", "insufficient_privilege"}, + {"3F000", "invalid_schema_name"}, + {"28P01", "invalid_password"}, + {"28000", "invalid_authorization"}, + {"3D000", "invalid_catalog_name"}, + } + for _, tc := range codes { + t.Run(tc.name, func(t *testing.T) { + hint := pgSetupHint(&pgconn.PgError{Code: tc.code}, "versus_incident") + if hint == "" { + t.Fatalf("pgSetupHint(%s) returned empty, want guide", tc.code) + } + if !strings.Contains(hint, "GRANT ALL ON SCHEMA public") { + t.Fatalf("pgSetupHint(%s) missing schema grant line: %q", tc.code, hint) + } + if !strings.Contains(hint, "versus_incident") { + t.Fatalf("pgSetupHint(%s) does not name the database: %q", tc.code, hint) + } + }) + } +} + +// TestPGSetupHint_FallbackDBName asserts the guide falls back to +// versus_enterprise when the DSN did not name a database. +func TestPGSetupHint_FallbackDBName(t *testing.T) { + hint := pgSetupHint(&pgconn.PgError{Code: "42501"}, "") + if !strings.Contains(hint, "versus_enterprise") { + t.Fatalf("pgSetupHint fallback did not name versus_enterprise: %q", hint) + } +} + +// TestPGSetupHint_ReturnsEmpty asserts no guide is emitted for a nil error, +// a non-permission PgError, or a plain (non-PgError) error. +func TestPGSetupHint_ReturnsEmpty(t *testing.T) { + if got := pgSetupHint(nil, "db"); got != "" { + t.Fatalf("pgSetupHint(nil) = %q, want empty", got) + } + // 08006 is connection_failure — not an auth/permission problem. + if got := pgSetupHint(&pgconn.PgError{Code: "08006"}, "db"); got != "" { + t.Fatalf("pgSetupHint(08006) = %q, want empty", got) + } + if got := pgSetupHint(errors.New("boom"), "db"); got != "" { + t.Fatalf("pgSetupHint(plain error) = %q, want empty", got) + } +} + +// TestPGSetupHint_NeverLeaksSecret asserts the static guide never contains a +// password or DSN — it is built from constant text plus the database name only. +func TestPGSetupHint_NeverLeaksSecret(t *testing.T) { + hint := pgSetupHint(&pgconn.PgError{ + Code: "28P01", + Message: "password authentication failed for user \"versus\"", + }, "versus_incident") + for _, secret := range []string{"s3cr3t", "postgres://", "password=", "@host"} { + if strings.Contains(hint, secret) { + t.Fatalf("pgSetupHint leaked %q: %q", secret, hint) + } + } +} + +// TestDSNDBName covers extracting the database name from both DSN shapes. +func TestDSNDBName(t *testing.T) { + tests := []struct { + dsn string + want string + }{ + {"postgres://versus:s3cr3t@host:5432/versus_incident?sslmode=require", "versus_incident"}, + {"host=host port=5432 dbname=versus_incident password=s3cr3t", "versus_incident"}, + {"host=host port=5432", ""}, + {"", ""}, + } + for _, tc := range tests { + if got := dsnDBName(tc.dsn); got != tc.want { + t.Fatalf("dsnDBName(%q) = %q, want %q", tc.dsn, got, tc.want) + } + } +} diff --git a/src/_sidebar.md b/src/_sidebar.md index c1c0b9b..dd2e343 100644 --- a/src/_sidebar.md +++ b/src/_sidebar.md @@ -53,6 +53,7 @@ - Configuration - [Overview](/configuration/admin-ui) - [Configuration](/configuration/configuration) + - [PostgreSQL Storage](/configuration/postgres-storage) - [Deploy on Kubernetes](/configuration/kubernetes) - [Helm Chart](/configuration/helm) diff --git a/src/agent/data-sources.md b/src/agent/data-sources.md index 3863452..0f3c1f3 100644 --- a/src/agent/data-sources.md +++ b/src/agent/data-sources.md @@ -48,6 +48,14 @@ Every source is cursor-based: This means restarts are safe: the agent picks up exactly where it left off, and no signal is processed twice or skipped. +> **Using a cluster-mode Redis/Valkey?** If cursor writes fail with repeated +> log lines like `agent: failed to persist cursor for : MOVED 14450 :6379`, +> your Redis has **cluster mode enabled** (e.g. AWS ElastiCache for Valkey/Redis) +> and shards keys across nodes. Set `redis.cluster: true` (or `REDIS_CLUSTER=true`) +> to build a cluster-aware client that follows those `MOVED` redirects. See +> [Cluster mode (Redis / Valkey)](../configuration/configuration.md#cluster-mode-redis--valkey) +> for details. A single primary/replicas setup does not need this flag. + ## Try it locally The runnable [docker-compose example](https://github.com/VersusControl/versus-incident/tree/main/examples/docker-compose) diff --git a/src/configuration/configuration.md b/src/configuration/configuration.md index 852bf08..2866498 100644 --- a/src/configuration/configuration.md +++ b/src/configuration/configuration.md @@ -168,6 +168,8 @@ redis: # Required for on-call functionality and the AI agent port: ${REDIS_PORT} password: ${REDIS_PASSWORD} db: 0 + # tls: true # default true; set false (or REDIS_TLS=false) for a plaintext dev Redis + # cluster: false # default false; set true (or REDIS_CLUSTER=true) for a cluster-mode Redis/Valkey # ----------------------------------------------------------------------------- # AI agent (training | shadow | detect) — opt-in. @@ -304,9 +306,12 @@ The application relies on several environment variables to configure alerting se | `GATEWAY_SECRET` | Shared secret required to access the admin dashboard and every `/api/admin/*` and `/api/agent/*` endpoint. Sent by clients in the `X-Gateway-Secret` header. **When unset the admin endpoints are not registered at all.** | ### Storage -| Variable | Description | -|--------------------------|-------------| -| `STORAGE_TYPE` | Storage backend for incidents and agent state. One of `file` (default and the only implemented backend today), `redis`, `database`. | +| Variable | Description | +|----------------|-------------| +| `STORAGE_TYPE` | Storage backend for incidents and agent state. One of `file` (default) or `postgres`. | +| `POSTGRES_DSN` | Postgres connection string — **required when `STORAGE_TYPE=postgres`**. e.g. `postgres://versus:your_strong_password@host:5432/versus_incident?sslmode=require`. Keep the password out of source control — set the DSN via env only. | + +> Using the PostgreSQL backend? See [PostgreSQL storage backend](/configuration/postgres-storage) for how to provision the database and role. ### Slack Configuration | Variable | Description | @@ -503,6 +508,32 @@ curl -X POST "http://localhost:3000/api/incidents?oncall_enable=true" \ | `REDIS_HOST` | The hostname or IP address of the Redis server. Required if on-call is enabled. | | `REDIS_PORT` | The port number of the Redis server. Required if on-call is enabled. | | `REDIS_PASSWORD` | The password for authenticating with the Redis server. Required if on-call is enabled and Redis requires authentication. | +| `REDIS_TLS` | Whether to dial Redis over TLS. **TLS is on by default** — only an explicit off-value (`false`/`0`/`no`/`off`, any case) disables it. Any other value keeps TLS on. | +| `REDIS_CA_CERT` | Path to a CA bundle used to verify the Redis server certificate when TLS is on. | +| `REDIS_CLUSTER` | Set to `true` to build a **cluster-aware** Redis client (see below). **Default is `false`** (single-node client — unchanged behaviour). Only `true`/`1`/`yes`/`on` (any case) enable cluster mode; any other value stays single-node. | + +#### Cluster mode (Redis / Valkey) + +By default Versus uses a **single-node** Redis client. Set `redis.cluster: true` +in `config.yaml` (or `REDIS_CLUSTER=true`) to build a **cluster-aware** client +instead. Turn this on when you point Versus at a managed Redis/Valkey running +with **cluster mode enabled** — e.g. AWS ElastiCache for Valkey/Redis with +cluster mode on — which shards keys across nodes and returns +`MOVED ` redirects that a single-node client cannot follow. + +The symptom this fixes is repeated log lines like: + +```text +agent: failed to persist cursor for my-source: MOVED 14450 clustercfg.example.amazonaws.com:6379 +``` + +A single primary/replicas deployment (**cluster mode disabled**) does **not** +need this flag — leave it `false`. + +Cluster mode **composes with the existing Redis settings** — `redis.tls` / +`REDIS_TLS`, `redis.password`, `redis.host` / `redis.port`, and `REDIS_CA_CERT` +all apply unchanged. In particular, the default-TLS behaviour is intact: TLS +stays on unless you set `REDIS_TLS=false`. ### AI Agent Configuration | Variable | Description | diff --git a/src/configuration/postgres-storage.md b/src/configuration/postgres-storage.md new file mode 100644 index 0000000..6fcb7ca --- /dev/null +++ b/src/configuration/postgres-storage.md @@ -0,0 +1,43 @@ +# PostgreSQL storage backend + +By default Versus Incident stores incidents and agent state on the local +filesystem (`STORAGE_TYPE=file`). For durable, shared, multi-replica storage, +switch the backend to PostgreSQL. + +## Enable Postgres + +Set two environment variables: + +| Variable | Description | +|----------------|-------------| +| `STORAGE_TYPE` | Set to `postgres` to use the PostgreSQL backend (default `file`). | +| `POSTGRES_DSN` | Postgres connection string — **required when `STORAGE_TYPE=postgres`**. Keep the password out of source control — set the DSN via env only. | + +Example DSN: + +``` +postgres://versus:your_strong_password@host:5432/versus_incident?sslmode=require +``` + +## Provision the database and role + +When `STORAGE_TYPE=postgres`, Versus runs its schema migrations +**automatically on boot**, so the role in `POSTGRES_DSN` must be able to +`CREATE` tables in the target database. Provision the database and role once as +a Postgres superuser (`psql`) before the first start: + +```sql +CREATE DATABASE versus_incident; +CREATE USER versus WITH PASSWORD 'your_strong_password'; +GRANT ALL PRIVILEGES ON DATABASE versus_incident TO versus; +GRANT ALL ON SCHEMA public TO versus; -- required on PostgreSQL 15+ +``` + +Substitute your own database name, username, and a strong password. + +- On **PostgreSQL 15+** the `public` schema no longer allows `CREATE` by + default. That final grant is the usual fix for a startup error like + `permission denied for schema public` during migration. +- The same failure now also prints this guidance to the container logs at + startup (detected from the Postgres SQLSTATE), but **this page is the + canonical reference** — follow the SQL here. diff --git a/ui/src/pages/PatternsPage.test.tsx b/ui/src/pages/PatternsPage.test.tsx index 43eaa36..f774092 100644 --- a/ui/src/pages/PatternsPage.test.tsx +++ b/ui/src/pages/PatternsPage.test.tsx @@ -151,3 +151,50 @@ describe("PatternsPage peek — fetches the pattern DETAIL", () => { expect(pre.className).not.toContain("overflow-auto"); }); }); + +// The Logs Verdict cell renders a learning hint next to the "Still learning" +// pill. Which hint it shows depends on whether count-based auto-promotion is +// enabled (readiness.needed): +// needed > 0 → a seen/needed progress meter ("40 / 100"). +// needed === 0 → auto-promotion is disabled, so the pattern never becomes +// known by count; the cell must say so and point the operator at the manual +// path, instead of showing a bare "Still learning" with no progress and no +// reason (the operator's "learning without progress" report). +describe("PatternsPage — Verdict cell learning hint", () => { + beforeEach(() => { + vi.mocked(api.listServiceOverrides).mockResolvedValue([]); + vi.mocked(api.getPattern).mockResolvedValue(detail()); + }); + + it("shows the seen/needed progress meter when a count target exists", async () => { + vi.mocked(api.listPatterns).mockResolvedValue([ + listRow({ + readiness: { ready: false, seen: 40, needed: 100, rate_per_min: 2 }, + }), + ]); + renderPage(); + const row = await screen.findByText("payment <*> failed"); + const cell = row.closest("tr")!; + expect(within(cell).getByRole("progressbar")).toBeTruthy(); + expect(cell.textContent).toContain("40"); + expect(cell.textContent).toContain("100"); + expect(within(cell).queryByText(/auto-promotion off/)).toBeNull(); + }); + + it("explains auto-promotion is off when there is no count target (needed === 0)", async () => { + vi.mocked(api.listPatterns).mockResolvedValue([ + listRow({ + count: 3304, + readiness: { ready: false, seen: 3304, needed: 0, rate_per_min: 12.6 }, + }), + ]); + renderPage(); + const row = await screen.findByText("payment <*> failed"); + const cell = row.closest("tr")!; + expect(within(cell).getByText(/auto-promotion off/)).toBeTruthy(); + expect(within(cell).getByText(/mark known by hand/)).toBeTruthy(); + // No progressbar meter for the indeterminate state. + expect(within(cell).queryByRole("progressbar")).toBeNull(); + }); +}); + diff --git a/ui/src/pages/PatternsPage.tsx b/ui/src/pages/PatternsPage.tsx index 0c60ee9..78251f8 100644 --- a/ui/src/pages/PatternsPage.tsx +++ b/ui/src/pages/PatternsPage.tsx @@ -533,6 +533,21 @@ export function PatternsPage() { )} + {p.verdict === "" && + p.readiness && + !p.readiness.ready && + p.readiness.needed === 0 && ( + + auto-promotion off + + {" "} + · mark known by hand + + + )} From 004e517699bbb5d5e772c335ec27da0a8bd9596a Mon Sep 17 00:00:00 2001 From: Quan Huynh Date: Wed, 8 Jul 2026 14:10:48 +0700 Subject: [PATCH 2/2] feat: enhance agent learning --- cmd/main.go | 2 +- cmd/main_test.go | 33 +++++++++ go.mod | 4 +- go.sum | 19 +++--- pkg/agent/brain_log.go | 58 +++++++++------- pkg/agent/brain_log_readiness_test.go | 41 ++++++++---- pkg/agent/brain_log_test.go | 67 +++++++++++-------- pkg/agent/catalog.go | 31 ++++++--- pkg/agent/catalog_pg_store.go | 17 ++++- pkg/agent/catalog_store_test.go | 46 +++++++++++++ pkg/agent/cursors.go | 2 +- pkg/agent/worker_seam_test.go | 57 +++++++++++----- pkg/config/agent.go | 10 ++- pkg/config/config.go | 10 +++ pkg/config/full_sample_test.go | 57 ++++++++++++++++ pkg/config/merge_test.go | 7 +- pkg/controllers/agent.go | 18 ++--- .../agent_patterns_readiness_test.go | 17 +++-- pkg/core/agent_signal.go | 6 +- pkg/core/oncall.go | 2 +- src/agent/agent-introduction.md | 3 +- src/agent/configuration.md | 4 +- ui/src/components/ReadinessProgress.tsx | 4 +- ui/src/lib/api.ts | 9 +-- ui/src/lib/readiness.test.ts | 6 +- ui/src/lib/readiness.ts | 5 +- ui/src/pages/PatternsPage.test.tsx | 26 +------ ui/src/pages/PatternsPage.tsx | 15 ----- 28 files changed, 390 insertions(+), 186 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index eb2125d..bfcc93d 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -25,7 +25,7 @@ import ( "github.com/VersusControl/versus-incident/pkg/teams" "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/ssmincidents" - "github.com/go-redis/redis/v8" + "github.com/redis/go-redis/v9" "github.com/VersusControl/versus-incident/pkg/common" diff --git a/cmd/main_test.go b/cmd/main_test.go index 9b11297..d1558f3 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -4,6 +4,7 @@ import ( "testing" c "github.com/VersusControl/versus-incident/pkg/config" + "github.com/redis/go-redis/v9" ) // TestHandlerRedisOptionsTLS covers the plaintext-Redis case: with TLS disabled the Redis @@ -41,3 +42,35 @@ func TestHandlerRedisOptionsTLS(t *testing.T) { } }) } + +// TestNewRedisClientClusterType verifies that enabling cluster mode builds a +// cluster-aware client (*redis.ClusterClient) rather than a single-node one. +// The cluster client is what parses the Redis 7 / Valkey CLUSTER SLOTS reply +// (which carries a 4th per-node metadata element) correctly, so cursor +// persistence keeps working against ElastiCache in cluster mode instead of +// falling back to in-memory. Both concrete clients are threaded as the shared +// redis.UniversalClient interface, which the assertions below also confirm. +func TestNewRedisClientClusterType(t *testing.T) { + tru := true + fls := false + + t.Run("cluster enabled returns *redis.ClusterClient", func(t *testing.T) { + client := newRedisClient(c.RedisConfig{Host: "localhost", Port: 6379, TLS: &fls, Cluster: &tru}) + defer client.Close() + + if _, ok := client.(*redis.ClusterClient); !ok { + t.Fatalf("expected *redis.ClusterClient when redis.cluster=true, got %T", client) + } + var _ redis.UniversalClient = client + }) + + t.Run("cluster disabled returns single-node *redis.Client", func(t *testing.T) { + client := newRedisClient(c.RedisConfig{Host: "localhost", Port: 6379, TLS: &fls}) + defer client.Close() + + if _, ok := client.(*redis.Client); !ok { + t.Fatalf("expected *redis.Client when cluster is off, got %T", client) + } + var _ redis.UniversalClient = client + }) +} diff --git a/go.mod b/go.mod index 505857a..38a7c93 100644 --- a/go.mod +++ b/go.mod @@ -16,10 +16,10 @@ require ( github.com/cloudwego/eino-ext/components/model/openai v0.1.13 github.com/cloudwego/eino-ext/components/model/qwen v0.1.9 github.com/go-git/go-git/v5 v5.19.1 - github.com/go-redis/redis/v8 v8.11.5 github.com/gofiber/fiber/v2 v2.52.14 github.com/google/uuid v1.6.0 github.com/jackc/pgx/v5 v5.10.0 + github.com/redis/go-redis/v9 v9.21.0 github.com/slack-go/slack v0.27.0 github.com/spf13/viper v1.21.0 golang.org/x/image v0.43.0 @@ -53,6 +53,7 @@ require ( go.opentelemetry.io/otel v1.29.0 // indirect go.opentelemetry.io/otel/metric v1.29.0 // indirect go.opentelemetry.io/otel/trace v1.29.0 // indirect + go.uber.org/atomic v1.11.0 // indirect golang.org/x/oauth2 v0.30.0 // indirect google.golang.org/api v0.197.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240903143218-8af14fe29dc1 // indirect @@ -131,7 +132,6 @@ require ( github.com/aws/aws-sdk-go-v2/service/sts v1.43.5 // indirect github.com/aws/smithy-go v1.27.3 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect - github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect github.com/go-viper/mapstructure/v2 v2.4.0 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect ) diff --git a/go.sum b/go.sum index 7da62f1..16e6347 100644 --- a/go.sum +++ b/go.sum @@ -64,6 +64,10 @@ github.com/bahlo/generic-list-go v0.2.0 h1:5sz/EEAK+ls5wF+NeqDpk5+iNdMDXrh3z3nPn github.com/bahlo/generic-list-go v0.2.0/go.mod h1:2KvAjgMlE5NNynlg/5iLrrCCZ2+5xWbdbCW3pNTGyYg= github.com/bitly/go-simplejson v0.5.0/go.mod h1:cXHtHw4XUPsvGaxgjIAn8PhEWG9NfngEKAMDJEczWVA= github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869/go.mod h1:Ekp36dRnpXw/yCqJaO+ZrUyxD+3VXMFFr56k5XYrpB4= +github.com/bsm/ginkgo/v2 v2.12.0 h1:Ny8MWAHyOepLGlLKYmXG4IEkioBysk6GpaRTLC8zwWs= +github.com/bsm/ginkgo/v2 v2.12.0/go.mod h1:SwYbGRRDovPVboqFv0tPTcG1sN61LM1Z4ARdbAV9g4c= +github.com/bsm/gomega v1.27.10 h1:yeMWxP2pV2fG3FgAODIY8EiRE3dy0aeFYt4l7wh6yKA= +github.com/bsm/gomega v1.27.10/go.mod h1:JyEr/xRbxbtgWNi8tIEVPUYZ5Dzef52k01W3YH0H+O0= github.com/buger/jsonparser v1.1.2 h1:frqHqw7otoVbk5M8LlE/L7HTnIq2v9RX6EJ48i9AxJk= github.com/buger/jsonparser v1.1.2/go.mod h1:6RYKKt7H4d4+iWqouImQ9R2FZql3VbhNgx27UK13J/0= github.com/bugsnag/bugsnag-go v1.4.0/go.mod h1:2oa8nejYd4cQ/b0hMIopN0lCRxU0bueqREvZLWFrtK8= @@ -115,8 +119,6 @@ github.com/cyphar/filepath-securejoin v0.6.1/go.mod h1:A8hd4EnAeyujCJRrICiOWqjS1 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f h1:lO4WD4F/rVNCu3HqELle0jiPLLBs70cWOduZpkS1E78= -github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f/go.mod h1:cuUVRXasLTGF7a8hSLbxyZXjz+1KgoB3wDUb6vlszIc= github.com/dnaeon/go-vcr v1.2.0 h1:zHCHvJYTMh1N7xnV7zf1m1GPBF9Ad0Jk/whtQ1663qI= github.com/dnaeon/go-vcr v1.2.0/go.mod h1:R4UdLID7HZT3taECzJs4YgbbH6PIGXB6W/sc5OLb6RQ= github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= @@ -160,8 +162,6 @@ github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY= github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= -github.com/go-redis/redis/v8 v8.11.5 h1:AcZZR7igkdvfVmQTPnu9WE37LRrO/YrBH5zWyjDC0oI= -github.com/go-redis/redis/v8 v8.11.5/go.mod h1:gREzHqY1hg6oD9ngVRbLStwAWKhA0FEgq8Jd4h5lpwo= github.com/go-test/deep v1.1.1 h1:0r/53hagsehfO4bzD2Pgr/+RgHqhmf+k1Bpse2cTu1U= github.com/go-test/deep v1.1.1/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE= github.com/go-viper/mapstructure/v2 v2.4.0 h1:EBsztssimR/CONLSZZ04E8qAkxNYq4Qp9LvH92wZUgs= @@ -260,14 +260,10 @@ github.com/modern-go/reflect2 v1.0.2 h1:xBagoLtFs94CBntxluKeaWgTMpvLxC4ur3nMaC9G github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= github.com/nikolalohinski/gonja v1.5.3 h1:GsA+EEaZDZPGJ8JtpeGN78jidhOlxeJROpqMT9fTj9c= github.com/nikolalohinski/gonja v1.5.3/go.mod h1:RmjwxNiXAEqcq1HeK5SSMmqFJvKOfTfXhkJv6YBtPa4= -github.com/nxadm/tail v1.4.8 h1:nPr65rt6Y5JFSKQO7qToXr7pePgD6Gwiw05lkbyAQTE= -github.com/nxadm/tail v1.4.8/go.mod h1:+ncqLTQzXmGhMZNUePPaPqPvBxHAIsmXswZKocGu+AU= github.com/ollama/ollama v0.20.3 h1:oP7eJZ+U4FzkoGGdzBq9mbq4wyuNyvVkz/BWxOrrAc0= github.com/ollama/ollama v0.20.3/go.mod h1:tCX4IMV8DHjl3zY0THxuEkpWDZSOchJpzTuLACpMwFw= github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/ginkgo v1.8.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= -github.com/onsi/ginkgo v1.16.5 h1:8xi0RTUf59SOSfEtZMvwTvXYMzG4gV23XVHOZiXNtnE= -github.com/onsi/ginkgo v1.16.5/go.mod h1:+E8gABHa3K6zRBolWtd+ROzc/U5bkGt0FwiG042wbpU= github.com/onsi/gomega v1.5.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= github.com/onsi/gomega v1.34.1 h1:EUMJIKUjM8sKjYbtxQI9A4z2o+rruxnzNvpknOXie6k= github.com/onsi/gomega v1.34.1/go.mod h1:kU1QgUvBDLXBJq618Xvm2LUX6rSAfRaFRTcdOeDLwwY= @@ -281,6 +277,8 @@ github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINE github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= +github.com/redis/go-redis/v9 v9.21.0 h1:FPBE4hhbAke+TLmcY3WkpbDffJEomdqPn3HYiqAtL9E= +github.com/redis/go-redis/v9 v9.21.0/go.mod h1:v/M13XI1PVCDcm01VtPFOADfZtHf8YW3baQf57KlIkA= github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= github.com/rivo/uniseg v0.4.7 h1:WUdvkW8uEhrYfLC4ZzdpI2ztxP1I582+49Oc5Mq64VQ= github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= @@ -359,6 +357,8 @@ github.com/xanzy/ssh-agent v0.3.3 h1:+/15pJfg/RsTxqYcX6fHqOXZwwMP+2VyYWJeWM2qQFM github.com/xanzy/ssh-agent v0.3.3/go.mod h1:6dzNDKs0J9rVPHPhaGCukekBHKqfl+L3KghI1Bc68Uw= github.com/yargevad/filepathx v1.0.0 h1:SYcT+N3tYGi+NvazubCNlvgIPbzAk7i7y2dwg3I5FYc= github.com/yargevad/filepathx v1.0.0/go.mod h1:BprfX/gpYNJHJfc35GjRRpVcwWXS89gGulUIU5tK3tA= +github.com/zeebo/xxh3 v1.1.0 h1:s7DLGDK45Dyfg7++yxI0khrfwq9661w9EN78eP/UZVs= +github.com/zeebo/xxh3 v1.1.0/go.mod h1:IisAie1LELR4xhVinxWS5+zf1lA4p0MW4T+w+W07F5s= go.opencensus.io v0.24.0 h1:y73uSU6J157QMP2kn2r30vwW1A2W2WFwSCGnAVxeaD0= go.opencensus.io v0.24.0/go.mod h1:vNK8G9p7aAivkbmorf4v+7Hgx+Zs0yY+0fOtgBfjQKo= go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.54.0 h1:r6I7RJCN86bpD/FQwedZ0vSixDpwuWREjW9oRMsmqDc= @@ -371,6 +371,8 @@ go.opentelemetry.io/otel/metric v1.29.0 h1:vPf/HFWTNkPu1aYeIsc98l4ktOQaL6LeSoeV2 go.opentelemetry.io/otel/metric v1.29.0/go.mod h1:auu/QWieFVWx+DmQOUMgj0F8LHWdgalxXqvp7BII/W8= go.opentelemetry.io/otel/trace v1.29.0 h1:J/8ZNK4XgR7a21DZUAsbF8pZ5Jcw1VhACmnYt39JTi4= go.opentelemetry.io/otel/trace v1.29.0/go.mod h1:eHl3w0sp3paPkYstJOmAimxhiFXPg+MMTlEh3nsQgWQ= +go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE= +go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0= go.uber.org/mock v0.4.0 h1:VcM4ZOtdbR4f6VXfiOpwpVJDL6lCReaZ6mw31wqh7KU= go.uber.org/mock v0.4.0/go.mod h1:a6FSlNadKUHUa9IP5Vyt1zh4fC7uAwxMutEAscFbkZc= go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= @@ -475,7 +477,6 @@ gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys= -gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw= gopkg.in/warnings.v0 v0.1.2 h1:wFXVbFY8DY5/xOe1ECiWdKCzZlxgshcYVNkBHstARME= gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI= diff --git a/pkg/agent/brain_log.go b/pkg/agent/brain_log.go index 07efba3..3b2a611 100644 --- a/pkg/agent/brain_log.go +++ b/pkg/agent/brain_log.go @@ -267,13 +267,13 @@ func (b *logBrain) Classify(obs core.Observation, mean, std float64, confident b prevCount := postCount - tickFreq // recover the pre-fold count prevVerdict := p.Verdict // Upsert never mutates Verdict, so == pre-fold - // AutoPromoteAfter ≤ 0 disables count-based promotion entirely ("0 disables - // the promotion"): a pattern is never marked "known" by sighting count - // alone, so it keeps flowing to detect-AI however often it is seen. The 100 - // default for an UNSET key is supplied by the embedded default_config layer - // (loaded as the base before user overrides), so an omitted key arrives here - // as 100 — only an explicit 0 (or negative) reaches the disabled branch. A - // pattern already promoted to "known" stays known regardless of threshold. + // The effective auto-promotion threshold gates count-based promotion: once + // a pattern's sighting count reaches it, the pattern is marked "known" and + // stops flowing to detect-AI on count alone. A non-positive configured + // value is resolved to the default by isLogKnown (there is no way to turn + // count-based promotion off), and an omitted key already arrives here as the + // default via the config layer. A pattern already promoted to "known" stays + // known regardless of threshold. threshold := b.cat.AutoPromoteAfter isKnown := isLogKnown(prevVerdict, postCount, threshold) if isKnown { @@ -340,9 +340,9 @@ func (b *logBrain) confirmSpike(key string, res spikeResult) bool { // training this is what keeps the Verdict column and the readiness "To known" // column in agreement (both derive from isLogKnown). // -// AutoPromoteAfter <= 0 disables count-based promotion entirely: for a not-yet- -// known pattern isLogKnown then returns false, so Promote never marks a pattern -// known by count in any mode; an operator-set "known" is untouched. +// A non-positive AutoPromoteAfter is resolved to the default by isLogKnown, so +// count-based promotion is always in effect; there is no "promotion disabled" +// state. func (b *logBrain) Promote(key string) { p := b.catalog.Get(key) if p == nil { @@ -353,16 +353,30 @@ func (b *logBrain) Promote(key string) { } } +// effectiveAutoPromote resolves the auto-promotion threshold used everywhere in +// the learning engine. A non-positive value (an operator who omitted, blanked, +// or zeroed the key, or a test that passes 0 directly) is treated as the +// default rather than as a "promotion disabled" state — there is no way to turn +// count-based promotion off. The config loader already normalizes the loaded +// config to a positive value; this is the belt-and-suspenders guard so a 0 that +// reaches the engine by any other path still resolves to a sane gate. +func effectiveAutoPromote(n int) int { + if n <= 0 { + return config.DefaultAutoPromoteAfter + } + return n +} + // isLogKnown is the SINGLE definition of log "known" — the exact predicate // logBrain.Classify gates promotion on. Both Classify and LogReadiness call it // so the classifier and the read-side readiness view can never drift; a drift // test pins them equal. A pattern is known when an operator (or a prior -// auto-promotion) already marked it "known", OR count-based promotion is -// enabled (autoPromoteAfter > 0) and the sighting count has reached it. -// autoPromoteAfter <= 0 disables count-based promotion, so only a pre-set -// "known" verdict counts. +// auto-promotion) already marked it "known", OR the sighting count has reached +// the effective auto-promotion threshold. A non-positive autoPromoteAfter is +// resolved to the default (see effectiveAutoPromote), so count-based promotion +// is always in effect. func isLogKnown(prevVerdict string, count, autoPromoteAfter int) bool { - return prevVerdict == "known" || (autoPromoteAfter > 0 && count >= autoPromoteAfter) + return prevVerdict == "known" || count >= effectiveAutoPromote(autoPromoteAfter) } // LogReadiness computes the readiness of one log pattern as a generic @@ -372,9 +386,9 @@ func isLogKnown(prevVerdict string, count, autoPromoteAfter int) bool { // gate compares) and the catalog's AutoPromoteAfter threshold. // // - Seen = the pattern's sighting count (the gate's own counter). -// - Needed = autoPromoteAfter when count-promotion is enabled (>0); 0 is the -// indeterminate sentinel when promotion is disabled (autoPromoteAfter<=0 → -// manual-only). +// - Needed = the effective auto-promotion threshold — always a positive gate +// (a non-positive configured value is resolved to the default), so there is +// no indeterminate/manual-only case. // - Ready = isLogKnown(...), so an already-"known"/"spike"-marked or // count-promoted pattern reports Ready. // - RatePerMin = the pattern's learned per-second sighting rate @@ -387,12 +401,10 @@ func LogReadiness(p *Pattern, autoPromoteAfter int, pollInterval time.Duration) return core.Readiness{} } r := core.Readiness{ - Seen: p.Count, - Ready: isLogKnown(p.Verdict, p.Count, autoPromoteAfter), + Seen: p.Count, + Needed: effectiveAutoPromote(autoPromoteAfter), + Ready: isLogKnown(p.Verdict, p.Count, autoPromoteAfter), } - if autoPromoteAfter > 0 { - r.Needed = autoPromoteAfter - } // else Needed=0 → indeterminate (manual-only promotion) if !r.Ready && pollInterval > 0 && p.BaselineFrequency > 0 { // BaselineFrequency is now a per-second sighting rate, so sightings/min // is just rate × 60 — poll-interval-independent. pollInterval is kept as diff --git a/pkg/agent/brain_log_readiness_test.go b/pkg/agent/brain_log_readiness_test.go index a013e33..65685cf 100644 --- a/pkg/agent/brain_log_readiness_test.go +++ b/pkg/agent/brain_log_readiness_test.go @@ -10,8 +10,8 @@ import ( // TestLogReadiness_DriftAgainstClassify is the anti-drift guard the design // requires. It drives a log pattern to a range of states across -// thresholds {below-default, at-default, below-custom, at-custom, disabled(0), -// disabled(negative), already-known} and asserts THREE views of "known" all +// thresholds {below-default, at-default, below-custom, at-custom, zero, +// negative, already-known} and asserts THREE views of "known" all // agree for every row: // // 1. isLogKnown(...) — the single extracted predicate @@ -75,7 +75,7 @@ func TestLogReadiness_DriftAgainstClassify(t *testing.T) { }, }, { - name: "auto-promote disabled (0), high count never known", + name: "zero threshold normalizes to default, high count becomes known", threshold: 0, build: func(t *testing.T, b *logBrain, c *Catalog) core.TypedVerdict { var v core.TypedVerdict @@ -86,7 +86,7 @@ func TestLogReadiness_DriftAgainstClassify(t *testing.T) { }, }, { - name: "auto-promote disabled (negative), high count never known", + name: "negative threshold normalizes to default, high count becomes known", threshold: -1, build: func(t *testing.T, b *logBrain, c *Catalog) core.TypedVerdict { var v core.TypedVerdict @@ -97,12 +97,12 @@ func TestLogReadiness_DriftAgainstClassify(t *testing.T) { }, }, { - name: "operator-marked known stays known even with promotion disabled", + name: "operator-marked known stays known regardless of threshold", threshold: 0, build: func(t *testing.T, b *logBrain, c *Catalog) core.TypedVerdict { // Create the pattern, hand-mark it known, then re-classify: the - // prevVerdict=="known" clause must win independently of the - // (disabled) count clause. + // prevVerdict=="known" clause must win independently of the count + // clause. classifyOnce(t, b, logObs("p", 1)) if !c.MarkKnown("p") { t.Fatalf("MarkKnown(p) returned false") @@ -171,22 +171,35 @@ func TestLogReadiness_EdgeCases(t *testing.T) { } }) - t.Run("AutoPromoteAfter<=0 → Needed=0 indeterminate, not ready", func(t *testing.T) { + t.Run("AutoPromoteAfter<=0 normalizes to the default gate", func(t *testing.T) { for _, threshold := range []int{0, -1, -100} { - p := &Pattern{Count: 500, BaselineFrequency: 0} // huge count, still not known + // A huge count is past the default gate, so a non-positive threshold + // (normalized to the default) makes the pattern ready. + p := &Pattern{Count: 500, BaselineFrequency: 0} r := LogReadiness(p, threshold, poll) - if r.Needed != 0 { - t.Errorf("threshold %d: Needed = %d, want 0 (indeterminate sentinel — manual-only)", threshold, r.Needed) + if r.Needed != config.DefaultAutoPromoteAfter { + t.Errorf("threshold %d: Needed = %d, want %d (normalized default)", threshold, r.Needed, config.DefaultAutoPromoteAfter) } - if r.Ready { - t.Errorf("threshold %d: Ready = true, want false (count promotion disabled, verdict empty)", threshold) + if !r.Ready { + t.Errorf("threshold %d: Ready = false, want true (count past the normalized default gate)", threshold) } if r.RatePerMin != 0 { - t.Errorf("threshold %d: RatePerMin = %v, want 0 (no baseline frequency yet)", threshold, r.RatePerMin) + t.Errorf("threshold %d: RatePerMin = %v, want 0 (Ready ⇒ no ETA)", threshold, r.RatePerMin) } } }) + t.Run("threshold 0 with count below default is not ready, Needed=default", func(t *testing.T) { + p := &Pattern{Count: 40, BaselineFrequency: 0} + r := LogReadiness(p, 0, poll) + if r.Needed != config.DefaultAutoPromoteAfter { + t.Errorf("Needed = %d, want %d (normalized default)", r.Needed, config.DefaultAutoPromoteAfter) + } + if r.Ready { + t.Errorf("Ready = true, want false (40 < %d)", config.DefaultAutoPromoteAfter) + } + }) + t.Run("already-known → Ready, RatePerMin=0 (no countdown)", func(t *testing.T) { // Even with a high BaselineFrequency, a Ready pattern reports RatePerMin=0 // because the ETA countdown only applies while still learning. diff --git a/pkg/agent/brain_log_test.go b/pkg/agent/brain_log_test.go index 26d5064..082ce59 100644 --- a/pkg/agent/brain_log_test.go +++ b/pkg/agent/brain_log_test.go @@ -302,33 +302,43 @@ func TestLogBrain_AutoPromoteAfter_CustomThresholdPromotes(t *testing.T) { } } -// (c) auto_promote_after: 0 DISABLES count-based promotion — a pattern -// is never marked "known" no matter how many times it is seen. Drives well past -// the old 100 fallback to prove the explicit 0 is honoured, not re-mapped. -func TestLogBrain_AutoPromoteAfter_ZeroDisablesPromotion(t *testing.T) { +// (c) auto_promote_after: 0 NORMALIZES to the default gate — a non-positive +// value is not a "disabled" state. Drive past the default 100 and the pattern +// becomes "known", exactly as it would with the default configured explicitly. +func TestLogBrain_AutoPromoteAfter_ZeroNormalizesToDefault(t *testing.T) { cat := config.AgentCatalogConfig{ - AutoPromoteAfter: 0, // documented: disables promotion + AutoPromoteAfter: 0, // non-positive → resolved to the default 100 SpikeMultiplier: 0, // disable spike so it can't mask the verdict } b, c := newLogBrainForTest(t, cat) + // Below the default gate: 9 × 10 = 90 sightings, not yet known. var v core.TypedVerdict - for i := 0; i < 12; i++ { - v = classifyOnce(t, b, logObs("p", 10)) // 120 sightings, well past 100 - } - if got := c.Get("p").Count; got != 120 { - t.Fatalf("count = %d, want 120", got) + for i := 0; i < 9; i++ { + v = classifyOnce(t, b, logObs("p", 10)) } if c.Get("p").Verdict == "known" { - t.Fatalf("auto_promote_after=0 promoted pattern to %q; 0 must disable promotion (QA-028)", c.Get("p").Verdict) + t.Fatalf("promoted early at count=90 (default gate is 100)") } if v.Class != core.VerdictUnknown { - t.Fatalf("verdict at count=120 = %v, want unknown (0 disables promotion)", v.Class) + t.Fatalf("verdict at count=90 = %v, want unknown (below the default gate)", v.Class) + } + + // Cross the default gate: 10th batch → 100 sightings, now known. + v = classifyOnce(t, b, logObs("p", 10)) + if got := c.Get("p").Count; got != 100 { + t.Fatalf("count = %d, want 100", got) + } + if c.Get("p").Verdict != "known" { + t.Fatalf("auto_promote_after=0 did not promote at count=100; <=0 must normalize to the default") + } + if v.Class != core.VerdictKnownPattern { + t.Fatalf("verdict at count=100 = %v, want known (0 normalizes to the default gate)", v.Class) } } -// (d) A negative threshold (any value ≤ 0) also disables promotion. -func TestLogBrain_AutoPromoteAfter_NegativeDisablesPromotion(t *testing.T) { +// (d) A negative threshold (any value ≤ 0) also normalizes to the default gate. +func TestLogBrain_AutoPromoteAfter_NegativeNormalizesToDefault(t *testing.T) { cat := config.AgentCatalogConfig{ AutoPromoteAfter: -1, SpikeMultiplier: 0, @@ -337,30 +347,29 @@ func TestLogBrain_AutoPromoteAfter_NegativeDisablesPromotion(t *testing.T) { var v core.TypedVerdict for i := 0; i < 12; i++ { - v = classifyOnce(t, b, logObs("p", 10)) // 120 sightings + v = classifyOnce(t, b, logObs("p", 10)) // 120 sightings, past the default 100 } - if c.Get("p").Verdict == "known" { - t.Fatalf("auto_promote_after=-1 promoted pattern to %q; any value ≤0 must disable promotion", c.Get("p").Verdict) + if c.Get("p").Verdict != "known" { + t.Fatalf("auto_promote_after=-1 did not promote at count=120; any value ≤0 must normalize to the default") } - if v.Class != core.VerdictUnknown { - t.Fatalf("verdict = %v, want unknown (negative disables promotion)", v.Class) + if v.Class != core.VerdictKnownPattern { + t.Fatalf("verdict = %v, want known (negative normalizes to the default gate)", v.Class) } } -// (e) An operator-labelled "known" pattern STAYS known even when count-based -// promotion is disabled (threshold 0). This bites the `prevVerdict == "known"` -// clause independently of the count clause: were the guard rewritten as -// `threshold > 0 && (prevVerdict == "known" || postCount >= threshold)`, a -// disabled threshold would silently un-suppress a hand-labelled pattern. +// (e) An operator-labelled "known" pattern STAYS known regardless of the +// effective count threshold. This bites the `prevVerdict == "known"` clause +// independently of the count clause: were the guard rewritten to require the +// count clause, a hand-labelled pattern below the gate would be un-suppressed. func TestLogBrain_AutoPromoteAfter_AlreadyKnownStaysKnown(t *testing.T) { cat := config.AgentCatalogConfig{ - AutoPromoteAfter: 0, // count-based promotion disabled + AutoPromoteAfter: 0, // non-positive → resolved to the default 100 SpikeMultiplier: 0, // disable spike so it can't mask the verdict } b, c := newLogBrainForTest(t, cat) - // Seed the pattern (stays Unknown — 0 disables count-based promotion), then - // label it known by hand as an operator would. + // Seed the pattern (stays Unknown — count 5 is far below the default gate), + // then label it known by hand as an operator would. classifyOnce(t, b, logObs("p", 5)) if !c.MarkKnown("p") { t.Fatalf("MarkKnown(p) did not mark the seeded pattern") @@ -370,10 +379,10 @@ func TestLogBrain_AutoPromoteAfter_AlreadyKnownStaysKnown(t *testing.T) { } // Further sightings must keep it known — the prior "known" verdict wins even - // though the count threshold is disabled. + // though the count is still below the gate. v := classifyOnce(t, b, logObs("p", 5)) if v.Class != core.VerdictKnownPattern { - t.Fatalf("already-known verdict = %v, want known (prior known must win with threshold 0)", v.Class) + t.Fatalf("already-known verdict = %v, want known (prior known must win below the gate)", v.Class) } if c.Get("p").Verdict != "known" { t.Fatalf("catalog verdict = %q, want known (an already-known pattern must stay known)", c.Get("p").Verdict) diff --git a/pkg/agent/catalog.go b/pkg/agent/catalog.go index 440780d..2f6618e 100644 --- a/pkg/agent/catalog.go +++ b/pkg/agent/catalog.go @@ -242,21 +242,30 @@ func (c *Catalog) Get(id string) *Pattern { // MarkKnown stamps a pattern as auto-promoted ("known") in the catalog. func (c *Catalog) MarkKnown(patternID string) bool { - if s := catalogStore(); s != nil { - return s.Curate(CatalogEdit{Kind: CatalogEditMarkKnown, PatternID: patternID}) == nil - } + // Always reflect the promotion in the in-memory working set so the + // single-pattern read (catalog.Get → the pattern detail page), the brain's + // verdict guard, and the Promote idempotency check see "known" immediately. + // When a store is installed its Snapshot backs the LIST view but NOT Get, so + // without this the detail page and the every-tick Promote guard would keep + // reading a stale empty verdict. c.mu.Lock() - defer c.mu.Unlock() p, ok := c.patterns[patternID] - if !ok { - return false + changed := ok && p.Verdict != "known" + if changed { + p.Verdict = "known" + c.dirty = true } - if p.Verdict == "known" { - return false + c.mu.Unlock() + + // When a durable store is installed, route the curation so the promotion is + // persisted (and applied fleet-wide). The store's MarkKnown creates the + // pattern's identity row if it does not exist yet, so a pattern that crossed + // auto_promote_after on its very first tick — before the debounced Persist has + // written it — is still promoted instead of silently lost. + if s := catalogStore(); s != nil { + return s.Curate(CatalogEdit{Kind: CatalogEditMarkKnown, PatternID: patternID}) == nil } - p.Verdict = "known" - c.dirty = true - return true + return changed } // RepointService immediately sets an EXISTING pattern's Service to service — diff --git a/pkg/agent/catalog_pg_store.go b/pkg/agent/catalog_pg_store.go index 740533e..acfd6ce 100644 --- a/pkg/agent/catalog_pg_store.go +++ b/pkg/agent/catalog_pg_store.go @@ -164,9 +164,20 @@ const ( WHERE p.org_id = $1 AND p.kind = 'log' AND p.deleted = FALSE` // Curate — one statement per operator mutation (all values bound). - sqlCurateVerdict = `UPDATE vs_patterns SET verdict = $3, updated_at = NOW() WHERE org_id = $1 AND id = $2 AND kind = 'log'` - sqlCurateTags = `UPDATE vs_patterns SET tags = $3, updated_at = NOW() WHERE org_id = $1 AND id = $2 AND kind = 'log'` - sqlCurateMarkKnown = `UPDATE vs_patterns SET verdict = 'known', updated_at = NOW() WHERE org_id = $1 AND id = $2 AND kind = 'log' AND COALESCE(verdict, '') <> 'known'` + sqlCurateVerdict = `UPDATE vs_patterns SET verdict = $3, updated_at = NOW() WHERE org_id = $1 AND id = $2 AND kind = 'log'` + sqlCurateTags = `UPDATE vs_patterns SET tags = $3, updated_at = NOW() WHERE org_id = $1 AND id = $2 AND kind = 'log'` + // MarkKnown must persist even when the pattern crossed auto_promote_after on + // its VERY FIRST tick — i.e. before the debounced Persist has written the + // pattern's identity row. An UPDATE would match 0 rows then and silently lose + // the promotion (worse: the caller caches it in markedKnown and never + // retries), so this UPSERTs the identity root: it creates the row with + // verdict='known' if absent, or promotes an existing one. Persist only ever + // updates `service` on conflict, so this curated verdict survives the later + // learned-row write and Snapshot then reports the pattern "known". + sqlCurateMarkKnown = `INSERT INTO vs_patterns (org_id, id, kind, verdict) + VALUES ($1, $2, 'log', 'known') + ON CONFLICT (org_id, id) DO UPDATE SET verdict = 'known', updated_at = NOW() + WHERE COALESCE(vs_patterns.verdict, '') <> 'known'` sqlCurateRepointService = `UPDATE vs_patterns SET service = $3, updated_at = NOW() WHERE org_id = $1 AND id = $2 AND kind = 'log'` sqlCurateDelete = `UPDATE vs_patterns SET deleted = TRUE, updated_at = NOW() WHERE org_id = $1 AND id = $2 AND kind = 'log'` sqlCurateResetPatterns = `DELETE FROM vs_patterns WHERE org_id = $1 AND kind = 'log'` diff --git a/pkg/agent/catalog_store_test.go b/pkg/agent/catalog_store_test.go index 2512292..8212237 100644 --- a/pkg/agent/catalog_store_test.go +++ b/pkg/agent/catalog_store_test.go @@ -208,6 +208,52 @@ func TestCatalog_InstalledStore_RoutesAtCallSites(t *testing.T) { } } +// TestCatalog_MarkKnown_MutatesInMemoryWithStore pins the fix for the +// "auto-promoted pattern still shows learning" bug: with a store installed, +// MarkKnown must ALSO stamp the in-memory pattern "known" (not only route the +// Curate), so the single-pattern read (catalog.Get → the detail page) and the +// Promote idempotency guard see the promotion immediately instead of a stale +// empty verdict. Regression: previously MarkKnown returned right after Curate +// and left the in-memory verdict blank, so the detail page rendered "-". +func TestCatalog_MarkKnown_MutatesInMemoryWithStore(t *testing.T) { + fake := &fakeCatalogStore{ + patterns: map[string]*Pattern{ + "p-hot": {ID: "p-hot", Template: "hot <*>", Count: 3304, Verdict: ""}, + }, + } + SetCatalogStore(fake) + t.Cleanup(func() { SetCatalogStore(nil) }) + + cat, err := LoadCatalog(nil) + if err != nil { + t.Fatalf("LoadCatalog: %v", err) + } + if p := cat.Get("p-hot"); p == nil || p.Verdict != "" { + t.Fatalf("precondition: p-hot should load with empty verdict, got %+v", p) + } + + cat.MarkKnown("p-hot") + + // In-memory read (the detail page path) now reflects the promotion. + if p := cat.Get("p-hot"); p == nil || p.Verdict != "known" { + t.Fatalf("after MarkKnown, in-memory verdict = %q, want \"known\" (detail page must not show '-')", verdictOf(p)) + } + // And the curation was still routed to the durable store. + fake.mu.Lock() + edits := append([]CatalogEdit(nil), fake.curates...) + fake.mu.Unlock() + if len(edits) != 1 || edits[0].Kind != CatalogEditMarkKnown || edits[0].PatternID != "p-hot" { + t.Fatalf("Curate edits = %+v, want one CatalogEditMarkKnown for p-hot", edits) + } +} + +func verdictOf(p *Pattern) string { + if p == nil { + return "" + } + return p.Verdict +} + // TestCatalog_InstalledStore_HotPathNeverCallsStore proves Get/Upsert and the // log brain's Expected stay in-memory and partition-local: they never consult // the store. diff --git a/pkg/agent/cursors.go b/pkg/agent/cursors.go index 8d2c7b2..d424545 100644 --- a/pkg/agent/cursors.go +++ b/pkg/agent/cursors.go @@ -5,7 +5,7 @@ import ( "sync" "time" - "github.com/go-redis/redis/v8" + "github.com/redis/go-redis/v9" ) // CursorStore persists the last-seen timestamp for each SignalSource. Redis diff --git a/pkg/agent/worker_seam_test.go b/pkg/agent/worker_seam_test.go index 85c53e7..5465f26 100644 --- a/pkg/agent/worker_seam_test.go +++ b/pkg/agent/worker_seam_test.go @@ -283,25 +283,46 @@ func TestWorker_Seam_TrainingPromotesVerdictToKnown(t *testing.T) { } } -// TestWorker_Seam_TrainingDisabledNeverPromotes proves auto_promote_after<=0 -// disables count-based promotion on the training learn path too: a pattern seen -// well past any default is never marked "known", and readiness stays not-ready -// — so the disabled contract holds in training, not just in shadow/detect. -func TestWorker_Seam_TrainingDisabledNeverPromotes(t *testing.T) { - src := &batchSource{name: "es", signals: repeatSignals("service=api chatter id=", 5)} - w, cat := newTrainingWorker(t, 0, src) // 0 disables count-based promotion +// TestWorker_Seam_TrainingZeroThresholdNormalizesAndPromotes proves a +// non-positive auto_promote_after is treated as the default (100) on the +// training learn path — there is no "promotion disabled" state. A pattern seen +// past the default IS marked "known" and readiness flips to ready, exactly as +// it would with the default configured explicitly. +func TestWorker_Seam_TrainingZeroThresholdNormalizesAndPromotes(t *testing.T) { + for _, threshold := range []int{0, -5} { + src := &batchSource{name: "es", signals: repeatSignals("service=api chatter id=", 5)} + w, cat := newTrainingWorker(t, threshold, src) // <=0 normalizes to the default 100 - for i := 0; i < 30; i++ { // 150 sightings, well past the 100 default + // Below the default: 19 ticks × 5 = 95 sightings, not yet known. + for i := 0; i < 19; i++ { + w.tickSource(context.Background(), src, "training") + } + p := singlePattern(t, cat) + if p.Count != 95 { + t.Fatalf("threshold=%d: count = %d, want 95", threshold, p.Count) + } + if p.Verdict == "known" { + t.Fatalf("threshold=%d: promoted early at count=95 (default gate is 100)", threshold) + } + if LogReadiness(p, threshold, 30*time.Second).Ready { + t.Fatalf("threshold=%d: readiness Ready below the default gate, want not ready", threshold) + } + + // Cross the default: 20th tick → 100 sightings, now known. w.tickSource(context.Background(), src, "training") - } - p := singlePattern(t, cat) - if p.Count != 150 { - t.Fatalf("count = %d, want 150", p.Count) - } - if p.Verdict == "known" { - t.Fatalf("auto_promote_after=0 promoted in training to %q; 0 must disable promotion", p.Verdict) - } - if LogReadiness(p, 0, 30*time.Second).Ready { - t.Fatalf("readiness Ready with promotion disabled, want not ready") + p = singlePattern(t, cat) + if p.Count != 100 { + t.Fatalf("threshold=%d: count = %d, want 100", threshold, p.Count) + } + if p.Verdict != "known" { + t.Fatalf("threshold=%d: count reached 100 but Verdict = %q, want %q "+ + "(<=0 must normalize to the default, not disable promotion)", threshold, p.Verdict, "known") + } + if !LogReadiness(p, threshold, 30*time.Second).Ready { + t.Fatalf("threshold=%d: readiness not Ready at the default gate, want ready", threshold) + } + if got := LogReadiness(p, threshold, 30*time.Second).Needed; got != 100 { + t.Fatalf("threshold=%d: readiness Needed = %d, want 100 (normalized default)", threshold, got) + } } } diff --git a/pkg/config/agent.go b/pkg/config/agent.go index 809a739..1e0f99d 100644 --- a/pkg/config/agent.go +++ b/pkg/config/agent.go @@ -4,6 +4,14 @@ package config // Agent mode (AI incident detection) // ----------------------------------------------------------------------------- +// DefaultAutoPromoteAfter is the sighting count at which a learned pattern is +// auto-promoted to "known" when the operator omits, blanks, or sets a +// non-positive auto_promote_after. It is the single source of truth for that +// default: the config loader normalizes any value <= 0 up to this, and the +// learning engine and readiness view fall back to it as well, so there is no +// "disabled / manual-only" promotion state. +const DefaultAutoPromoteAfter = 100 + type AgentConfig struct { Enable bool `mapstructure:"enable"` Mode string `mapstructure:"mode"` // training | shadow | detect @@ -55,7 +63,7 @@ type AgentRedactionConfig struct { type AgentCatalogConfig struct { PersistInterval string `mapstructure:"persist_interval"` // e.g. "30s" - AutoPromoteAfter int `mapstructure:"auto_promote_after"` // 0 = never + AutoPromoteAfter int `mapstructure:"auto_promote_after"` // sightings to "known"; <=0 is normalized to DefaultAutoPromoteAfter // SpikeZ is the z-score threshold: a known pattern is re-flagged as a spike // when its per-second rate sits this many standard deviations above the // learned baseline (self-scaling, so a burst on a high-volume pattern trips diff --git a/pkg/config/config.go b/pkg/config/config.go index 5cf0a63..b16edde 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -336,6 +336,16 @@ func loadConfigFromPath(path string) (*Config, error) { return nil, fmt.Errorf("failed to unmarshal config: %w", err) } + // Normalize the auto-promotion threshold at the single load chokepoint so + // every downstream consumer (worker brain, controller readiness, the + // /api/agent/config admin read) sees a positive gate. A present-but-empty + // key, a ${VAR} that expands to empty, or an explicit 0/negative all arrive + // here as <= 0; they are folded up to the default rather than becoming a + // silent "promotion disabled" state that leaves patterns learning forever. + if loaded.Agent.Catalog.AutoPromoteAfter <= 0 { + loaded.Agent.Catalog.AutoPromoteAfter = DefaultAutoPromoteAfter + } + setEnableFromEnv := func(envVar string, config *bool) { if value := os.Getenv(envVar); value != "" { *config = strings.ToLower(value) == "true" diff --git a/pkg/config/full_sample_test.go b/pkg/config/full_sample_test.go index 87c1e48..fdfc7c0 100644 --- a/pkg/config/full_sample_test.go +++ b/pkg/config/full_sample_test.go @@ -189,3 +189,60 @@ agent: } }) } + +// TestAutoPromoteAfterNormalization proves the load chokepoint folds any +// non-positive auto_promote_after up to the default: an explicit 0, a negative +// value, and an omitted key all arrive as the default, while a positive value +// passes through untouched. This is the single guard that stops a present-but- +// empty key or a ${VAR} that expands to empty from silently becoming a +// "promotion disabled" state. +func TestAutoPromoteAfterNormalization(t *testing.T) { + writeConfig := func(t *testing.T, body string) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + return path + } + + cases := []struct { + name string + yaml string + want int + }{ + { + name: "explicit zero normalizes to default", + yaml: "agent:\n catalog:\n auto_promote_after: 0\n", + want: DefaultAutoPromoteAfter, + }, + { + name: "negative normalizes to default", + yaml: "agent:\n catalog:\n auto_promote_after: -7\n", + want: DefaultAutoPromoteAfter, + }, + { + name: "omitted keeps default", + yaml: "agent:\n mode: training\n", + want: DefaultAutoPromoteAfter, + }, + { + name: "positive passes through unchanged", + yaml: "agent:\n catalog:\n auto_promote_after: 42\n", + want: 42, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + path := writeConfig(t, tc.yaml) + c, err := loadConfigFromPath(path) + if err != nil { + t.Fatalf("loadConfigFromPath: %v", err) + } + if c.Agent.Catalog.AutoPromoteAfter != tc.want { + t.Errorf("AutoPromoteAfter = %d, want %d", c.Agent.Catalog.AutoPromoteAfter, tc.want) + } + }) + } +} diff --git a/pkg/config/merge_test.go b/pkg/config/merge_test.go index b2aa31f..3cc891d 100644 --- a/pkg/config/merge_test.go +++ b/pkg/config/merge_test.go @@ -77,10 +77,9 @@ oncall: t.Errorf("Agent.Mode = %q, want training (default)", c.Agent.Mode) } // An OMITTED auto_promote_after must inherit the embedded default 100, NOT - // the zero value 0. Since 0 now DISABLES count-based promotion, an - // unset key that fell through to 0 would silently turn promotion off — this - // asserts unset ≠ disabled. + // the zero value 0. A non-positive value is normalized to the default at + // load, so an unset key still arrives as a positive gate. if c.Agent.Catalog.AutoPromoteAfter != 100 { - t.Errorf("Agent.Catalog.AutoPromoteAfter = %d, want 100 (embedded default for an omitted key; unset ≠ disabled)", c.Agent.Catalog.AutoPromoteAfter) + t.Errorf("Agent.Catalog.AutoPromoteAfter = %d, want 100 (embedded default for an omitted key)", c.Agent.Catalog.AutoPromoteAfter) } } diff --git a/pkg/controllers/agent.go b/pkg/controllers/agent.go index 6ee04c6..99cf3bc 100644 --- a/pkg/controllers/agent.go +++ b/pkg/controllers/agent.go @@ -66,8 +66,9 @@ type AgentController struct { // catalogCfg + pollInterval let listPatterns compute per-pattern readiness // (core.Readiness) without reaching into package globals. Wired at boot via // SetCatalogConfig from AgentConfig.Catalog + AgentConfig.PollInterval. Left - // unset (zero value), readiness degrades to Needed=0/RatePerMin=0 — the row - // still carries the field, just as "Learning" with no ETA (safe). + // unset (zero value), AutoPromoteAfter reads as 0 and LogReadiness resolves + // it to the default threshold, so the row still shows a sane target — just + // as "Learning" with no ETA (RatePerMin=0) until a worker is wired. catalogCfg config.AgentCatalogConfig pollInterval time.Duration } @@ -117,12 +118,13 @@ func (a *AgentController) SetSources(sources []core.SignalSource) *AgentControll // SetCatalogConfig wires the catalog threshold + worker poll interval into the // controller so listPatterns can attach a computed core.Readiness to each // pattern row WITHOUT reaching into package globals. `cat.AutoPromoteAfter` -// supplies the readiness `Needed` gate (≤0 → indeterminate/manual-only) and -// `poll` converts each pattern's per-tick sighting EWMA into a per-minute -// arrival rate for the ETA. Optional and safe to omit: left unset the readiness -// degrades to Needed=0/RatePerMin=0 (renders as "Learning", no ETA), so a -// process that runs no worker still serves well-formed rows. Returns the -// receiver for fluent wiring. +// supplies the readiness `Needed` gate — always a positive target, since +// LogReadiness resolves a non-positive value to the default — and `poll` +// converts each pattern's per-tick sighting EWMA into a per-minute arrival rate +// for the ETA. Optional and safe to omit: left unset the threshold reads as 0 +// and resolves to the default (renders as "Learning" toward that target, no +// ETA), so a process that runs no worker still serves well-formed rows. Returns +// the receiver for fluent wiring. func (a *AgentController) SetCatalogConfig(cat config.AgentCatalogConfig, poll time.Duration) *AgentController { a.catalogCfg = cat a.pollInterval = poll diff --git a/pkg/controllers/agent_patterns_readiness_test.go b/pkg/controllers/agent_patterns_readiness_test.go index 46db3bb..5ce0ab8 100644 --- a/pkg/controllers/agent_patterns_readiness_test.go +++ b/pkg/controllers/agent_patterns_readiness_test.go @@ -114,8 +114,10 @@ func TestListPatterns_CarriesReadiness(t *testing.T) { // TestListPatterns_UnsetCatalogConfigDegradesSafely proves that when no // worker/config is wired (SetCatalogConfig never called), readiness is still -// present and degrades to Needed=0/RatePerMin=0 (renders as "Learning", no ETA) -// rather than being absent or panicking. +// present: the zero-value AutoPromoteAfter is normalized to the default gate by +// LogReadiness, so Needed reads as the default (not zero) and RatePerMin=0 +// (renders as "Learning" toward that target, no ETA) rather than being absent +// or panicking. func TestListPatterns_UnsetCatalogConfigDegradesSafely(t *testing.T) { cat, err := agent.LoadCatalog(storage.NewMemory()) if err != nil { @@ -141,15 +143,16 @@ func TestListPatterns_UnsetCatalogConfigDegradesSafely(t *testing.T) { if r == nil { t.Fatal("readiness object missing even with unset config") } - if r["needed"] != float64(0) { - t.Errorf("readiness.needed = %v, want 0 (unset threshold → indeterminate)", r["needed"]) + if r["needed"] != float64(100) { + t.Errorf("readiness.needed = %v, want 100 (unset threshold normalizes to the default gate)", r["needed"]) } if r["rate_per_min"] != float64(0) { t.Errorf("readiness.rate_per_min = %v, want 0 (unset poll → no ETA)", r["rate_per_min"]) } - // Ready is false: threshold unset (0) disables count promotion and verdict is "". - if r["ready"] != false { - t.Errorf("readiness.ready = %v, want false (no count gate, verdict empty)", r["ready"]) + // count 150 >= the default gate 100, so an unwired controller now reports + // the pattern as ready rather than stuck learning forever. + if r["ready"] != true { + t.Errorf("readiness.ready = %v, want true (count past the default gate)", r["ready"]) } } diff --git a/pkg/core/agent_signal.go b/pkg/core/agent_signal.go index eac2946..3f9ad14 100644 --- a/pkg/core/agent_signal.go +++ b/pkg/core/agent_signal.go @@ -222,9 +222,9 @@ type Readiness struct { // Seen is the evidence folded so far (log sightings / folded samples). Seen int `json:"seen"` // Needed is the evidence required to reach Ready. 0 is the INDETERMINATE - // sentinel: no count gate applies (e.g. logs with AutoPromoteAfter<=0 — - // count-promotion is disabled, promotion is manual-only). The UI shows - // "Manual only", never "X of 0". + // sentinel: no count gate applies for this signal kind. The UI shows + // "Manual only", never "X of 0". (Log patterns always ship a positive + // Needed — a non-positive auto_promote_after is normalized to the default.) Needed int `json:"needed"` // RatePerMin is the observed arrival rate of NEW evidence for this key, // in evidence/minute, used to derive the ETA. 0 is the UNKNOWN/STALLED diff --git a/pkg/core/oncall.go b/pkg/core/oncall.go index 9fd58f7..6db6c19 100644 --- a/pkg/core/oncall.go +++ b/pkg/core/oncall.go @@ -9,7 +9,7 @@ import ( "github.com/VersusControl/versus-incident/pkg/config" "github.com/aws/aws-sdk-go-v2/service/ssmincidents" - "github.com/go-redis/redis/v8" + "github.com/redis/go-redis/v9" ) // OnCallProvider defines the interface for on-call notification providers diff --git a/src/agent/agent-introduction.md b/src/agent/agent-introduction.md index b80fc82..e0776e7 100644 --- a/src/agent/agent-introduction.md +++ b/src/agent/agent-introduction.md @@ -87,8 +87,7 @@ grouping is. The agent's long-term memory. Every template it learns is saved with: when it was first seen, when it was last seen, how many times it has appeared, an average rate, the filter rule that matched it, -and any labels you add. The catalog is saved to -`data/patterns.json`. +and any labels you add. The catalog is saved to backend storage. ### 6. The worker and modes The worker is the loop that runs everything on a timer. There are diff --git a/src/agent/configuration.md b/src/agent/configuration.md index ad7726e..c99ca80 100644 --- a/src/agent/configuration.md +++ b/src/agent/configuration.md @@ -127,8 +127,8 @@ catalog: | Key | Type | Default | Description | |---|---|---|---| -| `persist_interval` | duration | `30s` | How often the in-memory catalog is flushed to `data/patterns.json`. | -| `auto_promote_after` | int | `100` | A pattern seen this many times in `detect` mode is treated as known (won't alert). `0` disables the promotion. | +| `persist_interval` | duration | `30s` | How often the in-memory catalog is flushed to backend storage. | +| `auto_promote_after` | int | `100` | Number of sightings before a log pattern is auto-promoted to *known*. Default `100`. Any value `<= 0` (or a blank/unset key) is treated as the default `100`. To keep a pattern out of *known*, label/curate it instead. | The storage backend itself is selected at the **root** of `config.yaml` (`storage.type`), not here. The on-disk filename is fixed diff --git a/ui/src/components/ReadinessProgress.tsx b/ui/src/components/ReadinessProgress.tsx index 66b4f8f..d8a2305 100644 --- a/ui/src/components/ReadinessProgress.tsx +++ b/ui/src/components/ReadinessProgress.tsx @@ -7,7 +7,6 @@ import { deriveReadiness } from "@/lib/readiness"; // (matches the Logs Verdict cell). States: // ready → full bar + check + "Ready" // learning → accent bar + "seen / needed" -// indeterminate → seen count + "mark known by hand" (auto-promotion off) // absent → "—" // It reads the server Readiness facts via lib/readiness.ts so the number // stays consistent across every table, peek and detail surface. @@ -26,11 +25,12 @@ export function ReadinessProgress({ readiness }: { readiness?: Readiness }) { ); } + // Defensive: the server always ships a positive `needed`, but guard against a + // zero target (never expected) rather than dividing by zero into a NaN bar. if (d.indeterminate) { return ( {readiness.seen} seen - · mark known by hand ); } diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index 8b12f0f..52dccb7 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -125,13 +125,14 @@ async function request(path: string, init: RequestInit = {}): Promise { // settled/known state. It is present on every pattern and baseline row (logs // always; metrics/traces only where the enterprise brain runs). Presentation // (remaining, ETA, progress bar) is DERIVED by the UI from these facts — see -// lib/readiness.ts. Sentinels: needed === 0 ⇒ indeterminate (no count gate, -// e.g. logs with auto-promotion disabled); rate_per_min === 0 ⇒ no honest ETA -// (no rate yet / stalled / already ready). +// lib/readiness.ts. Log patterns always ship a positive `needed` (a non-positive +// auto_promote_after is normalized to the default upstream); `needed === 0` is a +// defensive sentinel only. rate_per_min === 0 ⇒ no honest ETA (no rate yet / +// stalled / already ready). export interface Readiness { ready: boolean; seen: number; - needed: number; // 0 ⇒ indeterminate (manual-only promotion) + needed: number; // always positive for logs; 0 is a defensive sentinel rate_per_min: number; // 0 ⇒ unknown/stalled ⇒ no ETA } diff --git a/ui/src/lib/readiness.test.ts b/ui/src/lib/readiness.test.ts index 683a9a6..cb2ff11 100644 --- a/ui/src/lib/readiness.test.ts +++ b/ui/src/lib/readiness.test.ts @@ -38,9 +38,11 @@ describe("deriveReadiness", () => { expect(d.etaMinutes).toBeNull(); }); - it("needed === 0 → indeterminate: no remaining/progress/eta", () => { + // Defensive only: the server always ships a positive `needed` (a non-positive + // auto_promote_after is normalized to the default upstream), but deriveReadiness + // must still stay sane if a zero target ever appears rather than dividing by zero. + it("needed === 0 → stays robust: no remaining/progress/eta", () => { const d = deriveReadiness(base({ seen: 5, needed: 0, rate_per_min: 3 })); - expect(d.state).toBe("indeterminate"); expect(d.indeterminate).toBe(true); expect(d.remaining).toBeNull(); expect(d.progress).toBeNull(); diff --git a/ui/src/lib/readiness.ts b/ui/src/lib/readiness.ts index 36609e1..203ed6e 100644 --- a/ui/src/lib/readiness.ts +++ b/ui/src/lib/readiness.ts @@ -24,7 +24,10 @@ export interface DerivedReadiness { // remaining = max(0, needed - seen) (needed > 0) // progress = seen / needed (needed > 0) // etaMinutes = remaining / rate_per_min (rate > 0, needed > 0, !ready) -// needed === 0 is the indeterminate sentinel; rate_per_min === 0 means no ETA. +// The server always ships a positive `needed` (a non-positive auto_promote_after +// is normalized to the default upstream), so `indeterminate` is a defensive +// guard for a zero target that is never expected in practice; rate_per_min === 0 +// means no ETA. export function deriveReadiness(r: Readiness): DerivedReadiness { const ready = r.ready; const hasTarget = r.needed > 0; diff --git a/ui/src/pages/PatternsPage.test.tsx b/ui/src/pages/PatternsPage.test.tsx index f774092..97bcc60 100644 --- a/ui/src/pages/PatternsPage.test.tsx +++ b/ui/src/pages/PatternsPage.test.tsx @@ -153,13 +153,9 @@ describe("PatternsPage peek — fetches the pattern DETAIL", () => { }); // The Logs Verdict cell renders a learning hint next to the "Still learning" -// pill. Which hint it shows depends on whether count-based auto-promotion is -// enabled (readiness.needed): -// needed > 0 → a seen/needed progress meter ("40 / 100"). -// needed === 0 → auto-promotion is disabled, so the pattern never becomes -// known by count; the cell must say so and point the operator at the manual -// path, instead of showing a bare "Still learning" with no progress and no -// reason (the operator's "learning without progress" report). +// pill: a seen/needed progress meter ("40 / 100"). The server always ships a +// positive `needed` (a non-positive auto_promote_after is normalized to the +// default upstream), so the cell always has a count target to render against. describe("PatternsPage — Verdict cell learning hint", () => { beforeEach(() => { vi.mocked(api.listServiceOverrides).mockResolvedValue([]); @@ -180,21 +176,5 @@ describe("PatternsPage — Verdict cell learning hint", () => { expect(cell.textContent).toContain("100"); expect(within(cell).queryByText(/auto-promotion off/)).toBeNull(); }); - - it("explains auto-promotion is off when there is no count target (needed === 0)", async () => { - vi.mocked(api.listPatterns).mockResolvedValue([ - listRow({ - count: 3304, - readiness: { ready: false, seen: 3304, needed: 0, rate_per_min: 12.6 }, - }), - ]); - renderPage(); - const row = await screen.findByText("payment <*> failed"); - const cell = row.closest("tr")!; - expect(within(cell).getByText(/auto-promotion off/)).toBeTruthy(); - expect(within(cell).getByText(/mark known by hand/)).toBeTruthy(); - // No progressbar meter for the indeterminate state. - expect(within(cell).queryByRole("progressbar")).toBeNull(); - }); }); diff --git a/ui/src/pages/PatternsPage.tsx b/ui/src/pages/PatternsPage.tsx index 78251f8..0c60ee9 100644 --- a/ui/src/pages/PatternsPage.tsx +++ b/ui/src/pages/PatternsPage.tsx @@ -533,21 +533,6 @@ export function PatternsPage() { )} - {p.verdict === "" && - p.readiness && - !p.readiness.ready && - p.readiness.needed === 0 && ( - - auto-promotion off - - {" "} - · mark known by hand - - - )}