diff --git a/cmd/migratecheck/README.md b/cmd/migratecheck/README.md new file mode 100644 index 0000000..9d467d2 --- /dev/null +++ b/cmd/migratecheck/README.md @@ -0,0 +1,29 @@ +cmd/migratecheck — run the real migration against a throwaway Postgres. + +The test suite never calls migrations.Migrate(): the integration tests +AutoMigrate only the models they need. So every raw statement at the end of +migrations/migrate.go — CREATE EXTENSION, the views, the indexes, the registry +seed — was unexecuted until this existed, and Migrate() logs those failures +NON-FATALLY. A broken view therefore returned "migration completed +successfully" and failed later at query time. + +That is not hypothetical: it caught `bd.destinationid` (the real column is +bookingdestinationid), which would have left consignment_booking uncreated and +every query that joins through it failing in production. + +Run it before shipping a migration change: + + docker run -d --name dm-testpg -e POSTGRES_PASSWORD=test -e POSTGRES_DB=logistics \ + -p 55432:5432 pgvector/pgvector:pg16 + docker exec dm-testpg psql -U postgres -d logistics -c "CREATE EXTENSION IF NOT EXISTS vector;" + + MIGRATE_CHECK_DSN="host=localhost port=55432 user=postgres password=test dbname=logistics sslmode=disable" \ + go run ./cmd/migratecheck + +Check the OUTPUT, not just the exit code — Migrate() returns OK on a logged +failure. Any ❌ or ⚠️ line is a statement that did not run. + +The same DSN unskips 50 Postgres integration tests: + + REGISTRY_TEST_DSN="host=localhost port=55432 user=postgres password=test dbname=logistics sslmode=disable" \ + go test ./... -count=1 diff --git a/cmd/migratecheck/main.go b/cmd/migratecheck/main.go new file mode 100644 index 0000000..39460cf --- /dev/null +++ b/cmd/migratecheck/main.go @@ -0,0 +1,40 @@ +// Runs migrations.Migrate() against a throwaway Postgres, so the DDL is +// executed at least once before it runs on a real database at server startup. +// +// The test suite never calls Migrate — the integration tests AutoMigrate the +// models they need — so every raw statement at the end of migrations/migrate.go +// (CREATE EXTENSION, the views, the CONCURRENTLY index, the seed) was +// unexecuted until this existed. +// +// Usage: MIGRATE_CHECK_DSN="host=... " go run ./cmd/migratecheck +package main + +import ( + "fmt" + "os" + + "doormile/db" + "doormile/migrations" + + "gorm.io/driver/postgres" + "gorm.io/gorm" +) + +func main() { + dsn := os.Getenv("MIGRATE_CHECK_DSN") + if dsn == "" { + fmt.Println("MIGRATE_CHECK_DSN is required (a THROWAWAY database)") + os.Exit(2) + } + gdb, err := gorm.Open(postgres.Open(dsn), &gorm.Config{}) + if err != nil { + fmt.Println("connect:", err) + os.Exit(1) + } + db.DB = gdb // the registry seed reads the package handle + if err := migrations.Migrate(gdb); err != nil { + fmt.Println("MIGRATE FAILED:", err) + os.Exit(1) + } + fmt.Println("MIGRATE RETURNED OK") +} diff --git a/config/config.go b/config/config.go index 69243f3..384466f 100644 --- a/config/config.go +++ b/config/config.go @@ -22,6 +22,12 @@ type Config struct { NatsUser string NatsPassword string AILayerBaseURL string // AI decision-engine service base URL (e.g. http://rider-api:8082) + // AIEngineBaseURL is AI_engine's own health surface (core/health.py), + // in-cluster. Distinct from AILayerBaseURL, which is routemate — the + // Claude-backed miler-selection service. Empty disables the agent-status + // proxy, which is the correct behaviour when the engine is not deployed: + // the console falls back to its snapshot rather than showing nothing. + AIEngineBaseURL string // RouteOptimizerURL is the Route Optimization API that orders a rider's // stops (Valhalla-backed road sequencing). Empty disables sequencing: stops @@ -83,7 +89,8 @@ func Load() *Config { NatsURL: getEnv("NATS_URL", "nats://66.116.226.161:4223"), NatsUser: getEnv("NATS_USER", "doormile"), NatsPassword: getEnv("NATS_PASSWORD", "Package@321#"), - AILayerBaseURL: getEnv("AI_LAYER_BASE_URL", "https://routemate.workolik.com"), + AILayerBaseURL: getEnv("AI_LAYER_BASE_URL", "https://routemate.workolik.com"), + AIEngineBaseURL: getEnv("AI_ENGINE_BASE_URL", ""), RouteOptimizerURL: getEnv("ROUTE_OPTIMIZER_URL", "https://routes.workolik.com"), GeocoderURL: getEnv("GEOCODER_URL", "https://nominatim.openstreetmap.org"), diff --git a/constants/delivery_category.go b/constants/delivery_category.go new file mode 100644 index 0000000..3850592 --- /dev/null +++ b/constants/delivery_category.go @@ -0,0 +1,56 @@ +package constants + +// What a client delivers, and what that implies operationally. +// +// ─── One vocabulary, not two ─────────────────────────────────────────────── +// +// These eight values already existed as `validCategories` in +// controllers/doormilePricingController.go and as the comment on +// models.DoormilePricing.Category. Tenants now carry one too, so the set is +// promoted here rather than copied: a second list would drift, and a tenant +// whose category is not a pricing category cannot be priced. +var DeliveryCategories = map[string]bool{ + "General": true, + "Documents": true, + "Electronics": true, + "Clothing": true, + "Fragile": true, + "Medical": true, + "Automotive": true, + "Food": true, +} + +// DeliveryCategoryList is the same set, ordered, for anything that renders a +// choice. General first because it is the safe default; Food last because it +// is the one that turns a capability off. +var DeliveryCategoryList = []string{ + "General", "Documents", "Electronics", "Clothing", + "Fragile", "Medical", "Automotive", "Food", +} + +// CategoryDefault is what a client with no category recorded is treated as. +// Every tenant onboarded before this field existed has an empty string, and +// they must keep behaving exactly as they did — which means reverse logistics +// stays available to them. +const CategoryDefault = "General" + +// ReverseLogisticsAllowed reports whether a return journey makes sense for +// what this client ships. +// +// Food is the exception: a meal that comes back is waste, not inventory. There +// is nothing to restock, nothing to refund against a returned item, and a +// rider carrying it to a hub is carrying rubbish. Offering RTO there is not a +// harmless extra button — it invites an operator to start a return journey +// that can only end in disposal, and it puts a return charge on a client's +// invoice for a parcel nobody can resell. +// +// Everything else can come back: clothing is the canonical case (wrong size), +// and electronics, documents and automotive parts all have a real return path. +// +// An UNKNOWN or empty category allows returns. That is deliberate: this field +// is new, every existing tenant has no value for it, and a default of "off" +// would silently withdraw a working capability from every client already using +// it. New information must not change old behaviour. +func ReverseLogisticsAllowed(category string) bool { + return category != "Food" +} diff --git a/constants/delivery_category_test.go b/constants/delivery_category_test.go new file mode 100644 index 0000000..0db5fb8 --- /dev/null +++ b/constants/delivery_category_test.go @@ -0,0 +1,52 @@ +package constants + +import "testing" + +// The rule the whole feature exists for: a returned meal is waste, not +// inventory, so Food clients get no reverse-logistics path. +func TestFoodIsTheOnlyCategoryWithoutReturns(t *testing.T) { + if ReverseLogisticsAllowed("Food") { + t.Error("Food allows returns; a returned meal can only be disposed of") + } + for _, c := range DeliveryCategoryList { + if c == "Food" { + continue + } + if !ReverseLogisticsAllowed(c) { + t.Errorf("%s does not allow returns; only Food should be excluded", c) + } + } +} + +// New information must not change old behaviour. Every tenant onboarded before +// this field existed has an empty category, and they were all using returns. +func TestUnknownAndEmptyCategoriesKeepReturns(t *testing.T) { + for _, c := range []string{"", " ", "Groceries", "SomethingNew"} { + if !ReverseLogisticsAllowed(c) { + t.Errorf("category %q withdrew returns; an unrecognised category must not "+ + "silently remove a capability an existing client is already using", c) + } + } +} + +// The tenant category and the pricing category must stay ONE vocabulary: a +// tenant whose category is not a pricing category cannot be priced. +func TestListAndSetAgree(t *testing.T) { + if len(DeliveryCategoryList) != len(DeliveryCategories) { + t.Fatalf("list has %d, set has %d", len(DeliveryCategoryList), len(DeliveryCategories)) + } + for _, c := range DeliveryCategoryList { + if !DeliveryCategories[c] { + t.Errorf("%s is in the list but not the set", c) + } + } +} + +func TestDefaultIsAValidCategoryThatAllowsReturns(t *testing.T) { + if !DeliveryCategories[CategoryDefault] { + t.Errorf("CategoryDefault %q is not a valid category", CategoryDefault) + } + if !ReverseLogisticsAllowed(CategoryDefault) { + t.Error("the default category must not withdraw returns") + } +} diff --git a/controllers/agentDecisionController.go b/controllers/agentDecisionController.go index 9e3d270..2c197ea 100644 --- a/controllers/agentDecisionController.go +++ b/controllers/agentDecisionController.go @@ -21,6 +21,7 @@ func CreateAgentDecision(c *fiber.Ctx) error { Context map[string]interface{} `json:"context"` Decision map[string]interface{} `json:"decision"` Reasoning string `json:"reasoning"` + TenantID *uint64 `json:"tenant_id"` ContextEmbedding []float32 `json:"context_embedding"` } @@ -44,6 +45,7 @@ func CreateAgentDecision(c *fiber.Ctx) error { record := models.AgentDecision{ DecisionType: body.DecisionType, BookingID: body.BookingID, + TenantID: body.TenantID, Context: string(contextJSON), Decision: string(decisionJSON), Reasoning: body.Reasoning, @@ -73,6 +75,10 @@ func CreateAgentDecision(c *fiber.Ctx) error { } // GET /api/v1/internal/agent-decisions/similar +// POST /api/v1/internal/agent-decisions/similar +// +// POST rather than GET because the body carries a 1536-float embedding, and a +// GET body does not survive nginx — which fronts this API today. func FindSimilarDecisions(c *fiber.Ctx) error { decisionType := c.Query("decision_type") limit, err := strconv.Atoi(c.Query("limit", "5")) @@ -82,6 +88,11 @@ func FindSimilarDecisions(c *fiber.Ctx) error { type req struct { Embedding []float32 `json:"embedding"` + // TenantID scopes the search. Omitted or null means "decisions with no + // tenant" — NOT "every tenant". Precedent must never cross tenants: + // the engine feeds these rows to the model, so one client's history + // would otherwise shape another client's dispatch. + TenantID *uint64 `json:"tenant_id"` } body := new(req) if err := c.BodyParser(body); err != nil || len(body.Embedding) == 0 { @@ -102,14 +113,24 @@ func FindSimilarDecisions(c *fiber.Ctx) error { } var results []row + // outcome IS NOT NULL is the point of the table: an unresolved decision is + // not precedent, it is just a past guess. The outcome sweeper + // (internal/ai/outcomes) is what makes rows eligible here. + // + // The tenant predicate uses IS NOT DISTINCT FROM so a NULL tenant matches + // only NULL — plain `= ?` would match nothing at all for B2C decisions and + // silently return an empty recall forever. if err := db.DB.Raw( `SELECT decision, reasoning, outcome, context_embedding <=> ? AS distance FROM agent_decisions - WHERE decision_type = ? AND outcome IS NOT NULL + WHERE decision_type = ? + AND outcome IS NOT NULL + AND context_embedding IS NOT NULL + AND tenantid IS NOT DISTINCT FROM ? ORDER BY context_embedding <=> ? LIMIT ?`, - embeddingStr, decisionType, embeddingStr, limit, + embeddingStr, decisionType, body.TenantID, embeddingStr, limit, ).Scan(&results).Error; err != nil { utils.Error("FindSimilarDecisions: query failed", "error", err) return utils.Internal(c, "failed to query similar decisions") diff --git a/controllers/aiEngineProxyController.go b/controllers/aiEngineProxyController.go new file mode 100644 index 0000000..f9a9922 --- /dev/null +++ b/controllers/aiEngineProxyController.go @@ -0,0 +1,87 @@ +package controllers + +import ( + "encoding/json" + "io" + "net/http" + "time" + + "doormile/utils" + + "github.com/gofiber/fiber/v2" +) + +// Proxying AI_engine's live agent state to the console. +// +// The console's Agents page has run on a hand-maintained snapshot +// (krow_talent_app/src/lib/agentNetwork.js, dated 16-20 Sep) because the engine +// exposed no endpoint it could read. The engine now serves GET /agents/status +// from core/health.py — but the console cannot call it directly: that surface +// is a ClusterIP Service on port 8700, and the console is a browser. +// +// So this proxies it. Staff-only, like the rest of /admin/ai. +// +// ─── Fails soft, on purpose ──────────────────────────────────────────────── +// +// A 503 here means "ask the snapshot", not "something is broken". The engine is +// not deployed in Kubernetes yet, AI_ENGINE_BASE_URL is empty by default, and +// the Agents page must keep rendering in all of those cases. The console reads +// the status code and falls back; it never shows an error for this. +// +// Short timeout for the same reason: an unreachable engine must not hold a +// console request open. Two seconds is longer than an in-cluster call needs and +// shorter than anyone will wait. + +// AIEngineBaseURL is set at boot from config. Empty disables the proxy. +var AIEngineBaseURL string + +var engineClient = &http.Client{Timeout: 2 * time.Second} + +// GET /api/v1/admin/ai/engine/agents +func GetAIEngineAgents(c *fiber.Ctx) error { + if AIEngineBaseURL == "" { + // Not an error: the engine has no in-cluster address configured, which + // is the default and is true today. The console falls back. + return c.Status(fiber.StatusServiceUnavailable).JSON(fiber.Map{ + "code": "AI_ENGINE_NOT_CONFIGURED", + "message": "AI_ENGINE_BASE_URL is not set; the console should use its snapshot.", + }) + } + + resp, err := engineClient.Get(AIEngineBaseURL + "/agents/status") + if err != nil { + utils.Warn("GetAIEngineAgents: engine unreachable", "error", err) + return c.Status(fiber.StatusServiceUnavailable).JSON(fiber.Map{ + "code": "AI_ENGINE_UNREACHABLE", + "message": "The engine did not answer; the console should use its snapshot.", + }) + } + defer resp.Body.Close() + + // Cap the read. This is a trusted in-cluster service, but a proxy that + // streams an unbounded body into a console response is a bad shape + // regardless of who is on the other end. + body, err := io.ReadAll(io.LimitReader(resp.Body, 256*1024)) + if err != nil { + return c.Status(fiber.StatusServiceUnavailable).JSON(fiber.Map{ + "code": "AI_ENGINE_UNREADABLE", "message": "Could not read the engine's response.", + }) + } + if resp.StatusCode != http.StatusOK { + // The engine answers 503 on /agents/status when it has no agent state + // (a non-production run mode). Pass that through rather than + // reinterpreting it. + return c.Status(fiber.StatusServiceUnavailable).JSON(fiber.Map{ + "code": "AI_ENGINE_NO_STATE", + "message": "The engine is running but reports no agent state.", + }) + } + + var payload map[string]interface{} + if err := json.Unmarshal(body, &payload); err != nil { + return c.Status(fiber.StatusServiceUnavailable).JSON(fiber.Map{ + "code": "AI_ENGINE_BAD_JSON", "message": "The engine's response was not JSON.", + }) + } + return utils.OK(c, payload) +} diff --git a/controllers/aiFindingController.go b/controllers/aiFindingController.go new file mode 100644 index 0000000..24c694c --- /dev/null +++ b/controllers/aiFindingController.go @@ -0,0 +1,287 @@ +package controllers + +import ( + "encoding/json" + "net/url" + "strconv" + + "doormile/db" + "doormile/models" + "doormile/utils" + + "github.com/gofiber/fiber/v2" + "gorm.io/gorm" + "gorm.io/gorm/clause" +) + +// Skill findings: the console's rule output, made durable. +// +// See models/ai_findings.go for why this is neither an exception nor a decision. +// The short version: the eight ops skills run in the browser and threw their +// output away, so nobody could answer whether a skill was useful, whether a +// finding had been seen before, or whether acting on one actually cleared it. + +// POST /api/v1/admin/ai/findings +// +// Idempotent on fingerprint. The console re-evaluates every 60 seconds and will +// re-send an unchanged finding each time; this bumps lastseenat and seencount +// rather than inserting a duplicate. Without that, the table would grow by the +// number of open findings per minute per operator with the Exceptions page +// open — and "how long has this been open" would be unanswerable, which is the +// one signal that distinguishes an ignored finding from a new one. +func UpsertAIFindings(c *fiber.Ctx) error { + type item struct { + Skillid string `json:"skillid"` + Fingerprint string `json:"fingerprint"` + Severity string `json:"severity"` + Title string `json:"title"` + Proposaltool string `json:"proposaltool"` + Bookingids []int `json:"bookingids"` + Tenantid *int `json:"tenantid"` + } + var body struct { + Findings []item `json:"findings"` + // Cleared carries the fingerprints a scan did NOT raise this time. + // Sent by the console because only it knows the full set it evaluated: + // the backend cannot distinguish "resolved" from "the operator closed + // the tab" on its own. + Cleared []string `json:"cleared"` + } + if err := c.BodyParser(&body); err != nil { + return utils.BadRequest(c, "invalid request body") + } + + now := utils.DBNow() + written, skipped := 0, 0 + + for _, f := range body.Findings { + if f.Skillid == "" || f.Fingerprint == "" { + skipped++ + continue + } + scopeJSON, err := json.Marshal(f.Bookingids) + if err != nil { + skipped++ + continue + } + + row := models.AISkillFinding{ + Skillid: f.Skillid, + Fingerprint: f.Fingerprint, + Severity: f.Severity, + Title: f.Title, + Proposaltool: f.Proposaltool, + Bookingcount: len(f.Bookingids), + Scope: string(scopeJSON), + Tenantid: f.Tenantid, + Firstseenat: now, + Lastseenat: now, + Seencount: 1, + } + + // ON CONFLICT on the fingerprint: bump the sighting, leave firstseenat + // alone. seencount uses a SQL expression rather than a read-modify-write + // so two operators with the page open cannot lose each other's bump. + // + // clearedat is reset to NULL: a finding that cleared and came back is + // open again, and leaving the old timestamp would make it look resolved + // while it is being re-raised. + if err := db.DB.Clauses(clause.OnConflict{ + Columns: []clause.Column{{Name: "fingerprint"}}, + DoUpdates: clause.Assignments(map[string]interface{}{ + "lastseenat": now, + "seencount": gorm.Expr("aiskillfindings.seencount + 1"), + "severity": f.Severity, + "bookingcount": len(f.Bookingids), + "scope": string(scopeJSON), + "clearedat": nil, + }), + }).Create(&row).Error; err != nil { + utils.Warn("UpsertAIFindings: upsert failed", "fingerprint", f.Fingerprint, "error", err) + skipped++ + continue + } + written++ + } + + cleared := 0 + if len(body.Cleared) > 0 { + // Only clear what is still open. Re-clearing an already-cleared row + // would move its clearedat forward on every poll and destroy the + // "acting cleared it in 4 minutes" measurement. + res := db.DB.Model(&models.AISkillFinding{}). + Where("fingerprint IN ? AND clearedat IS NULL", body.Cleared). + Update("clearedat", now) + if res.Error != nil { + utils.Warn("UpsertAIFindings: clearing failed", "error", res.Error) + } else { + cleared = int(res.RowsAffected) + } + } + + return utils.OK(c, fiber.Map{"written": written, "skipped": skipped, "cleared": cleared}) +} + +// POST /api/v1/admin/ai/findings/:fingerprint/acted +// +// Records that an operator carried out a finding's proposal, and how it went. +// Separate from the upsert because it is a different event with a different +// actor: the upsert is a scan reporting what it sees, this is a person doing +// something. Collapsing them would make "nobody acted" indistinguishable from +// "the scan has not run since". +func RecordAIFindingActed(c *fiber.Ctx) error { + // Fiber's c.Params returns the RAW path segment, still percent-encoded. + // A fingerprint is "skill:tool:1,2,3", so the console necessarily sends it + // through encodeURIComponent and the ':' and ',' arrive as %3A and %2C. + // Matching the raw string against the stored one therefore never hits, and + // every acted-report 404s — silently, because the console treats this call + // as fire-and-forget. + // + // Found by running the real backend; a mock that echoed the path back + // agreed with the assumption and proved nothing. + fingerprint, err := url.PathUnescape(c.Params("fingerprint")) + if err != nil { + return utils.BadRequest(c, "invalid fingerprint") + } + if fingerprint == "" { + return utils.BadRequest(c, "fingerprint is required") + } + var body struct { + Result string `json:"result"` // ok, partial, failed + } + if err := c.BodyParser(&body); err != nil { + return utils.BadRequest(c, "invalid request body") + } + switch body.Result { + case "ok", "partial", "failed": + default: + // A partial success is a partial success — the console reports six + // riders notified out of eight that way, and flattening it to "ok" + // here would lose exactly the distinction the executors preserve. + return utils.BadRequest(c, "result must be one of: ok, partial, failed") + } + + actor, _ := c.Locals("userid").(int) + now := utils.DBNow() + + res := db.DB.Model(&models.AISkillFinding{}). + Where("fingerprint = ?", fingerprint). + Updates(map[string]interface{}{ + "actedat": now, + "actedby": actor, + "actionresult": body.Result, + }) + if res.Error != nil { + utils.Error("RecordAIFindingActed: update failed", "error", res.Error) + return utils.Internal(c, "failed to record the action") + } + if res.RowsAffected == 0 { + return utils.NotFound(c, "finding not found") + } + return utils.OK(c, fiber.Map{"fingerprint": fingerprint, "result": body.Result}) +} + +// GET /api/v1/admin/ai/findings +// +// What the skills have been noticing. Read-only; Doormile staff only, like the +// rest of the /admin/ai surface. +// +// Defaults to open findings (clearedat IS NULL) because that is the operational +// question. `?days=N` switches to everything in a window, which is the +// measurement question — and those are different enough that one default cannot +// serve both. +func GetAIFindings(c *fiber.Ctx) error { + limit, err := strconv.Atoi(c.Query("limit", "100")) + if err != nil || limit < 1 || limit > 500 { + limit = 100 + } + + q := db.DB.Model(&models.AISkillFinding{}) + if days, err := strconv.Atoi(c.Query("days", "0")); err == nil && days > 0 { + if days > 90 { + days = 90 + } + q = q.Where("firstseenat >= ?", utils.DBNow().AddDate(0, 0, -days)) + } else { + q = q.Where("clearedat IS NULL") + } + if skill := c.Query("skill"); skill != "" { + q = q.Where("skillid = ?", skill) + } + + var rows []models.AISkillFinding + if err := q.Order("lastseenat DESC").Limit(limit).Find(&rows).Error; err != nil { + utils.Error("GetAIFindings: query failed", "error", err) + return utils.Internal(c, "failed to read findings") + } + return utils.List(c, rows, int64(len(rows))) +} + +// GET /api/v1/admin/ai/findings/stats +// +// Per skill, over a window: how often it fires, how long its findings stay +// open, how often anyone acts, and whether acting cleared them. +// +// This is the point of the table. "Acted and cleared" versus "cleared on its +// own" is what separates a skill that helps from one that narrates — and a +// skill whose findings always clear untouched is proposing work that did not +// need doing. +func GetAIFindingStats(c *fiber.Ctx) error { + days, err := strconv.Atoi(c.Query("days", "30")) + if err != nil || days < 1 || days > 90 { + days = 30 + } + since := utils.DBNow().AddDate(0, 0, -days) + + type stat struct { + Skillid string `json:"skillid"` + Findings int64 `json:"findings"` + Stillopen int64 `json:"stillopen"` + Actedon int64 `json:"actedon"` + Clearedafteract int64 `json:"clearedafteract"` + Clearedunacted int64 `json:"clearedunacted"` + Avgopenminutes *float64 `json:"avgopenminutes"` + } + var rows []stat + + // EXTRACT over (clearedat - firstseenat): both are written with + // utils.DBNow, so they share a tagging and their difference is correct + // regardless of the IST-digits-labelled-UTC convention. + if err := db.DB.Raw(` + SELECT skillid, + count(*) AS findings, + count(*) FILTER (WHERE clearedat IS NULL) AS stillopen, + count(*) FILTER (WHERE actedat IS NOT NULL) AS actedon, + count(*) FILTER (WHERE actedat IS NOT NULL AND clearedat IS NOT NULL) AS clearedafteract, + count(*) FILTER (WHERE actedat IS NULL AND clearedat IS NOT NULL) AS clearedunacted, + avg(EXTRACT(EPOCH FROM (clearedat - firstseenat)) / 60.0) + FILTER (WHERE clearedat IS NOT NULL) AS avgopenminutes + FROM aiskillfindings + WHERE firstseenat >= ? + GROUP BY skillid + ORDER BY findings DESC`, since).Scan(&rows).Error; err != nil { + utils.Error("GetAIFindingStats: query failed", "error", err) + return utils.Internal(c, "failed to read finding stats") + } + + return utils.OK(c, fiber.Map{ + "days": days, + "since": since, + "skills": rows, + }) +} + +// PruneAIFindings drops findings past the retention window. Called from the +// outcome sweeper's tick rather than having its own timer — one more table to +// keep tidy, not one more goroutine. +func PruneAIFindings(retentionDays int) (int, error) { + if db.DB == nil || retentionDays <= 0 { + return 0, nil + } + cutoff := utils.DBNow().AddDate(0, 0, -retentionDays) + res := db.DB.Where("firstseenat < ?", cutoff).Delete(&models.AISkillFinding{}) + if res.Error != nil { + return 0, res.Error + } + return int(res.RowsAffected), nil +} diff --git a/controllers/batchAssignService.go b/controllers/batchAssignService.go new file mode 100644 index 0000000..89d46fe --- /dev/null +++ b/controllers/batchAssignService.go @@ -0,0 +1,266 @@ +package controllers + +import ( + "math" + + "doormile/constants" + "doormile/db" + "doormile/internal/routing" + "doormile/models" + "doormile/utils" + + "github.com/gofiber/fiber/v2" + "gorm.io/gorm" +) + +// The shared batch-assign solver. +// +// This was the body of HubBatchAssign. It is lifted out so the admin console +// can run the same solver, because the console's agent layer needs it and the +// hub route is unreachable from an admin login. +// +// ─── Why the console could not use any existing route ────────────────────── +// +// The ops-layer skills raise findings that propose assigning a set of orders +// (SlaGuardianSkill's `assignMiler`), and that proposal has had no executor at +// all. The two candidate routes both failed, for different reasons: +// +// POST /hub/bookings/batch-assign sits behind HubStaffAuth, which refuses +// every token whose role is not 6. From the +// admin console it 403s on every click. +// POST /admin/bookings/:id/assign-miler +// needs a CHOSEN rider per booking +// ({mileruserid}), and a finding does not +// pick one — it names orders, not riders. +// +// So the console needed the hub route's SOLVER (which picks riders itself) with +// the admin route's AUTH. Extracting the solver gives both routes one +// implementation; a second copy would be a third definition of "who gets this +// booking", after internal/assignment's AI path and AssignMilerToBooking. +// +// ─── What this is NOT ────────────────────────────────────────────────────── +// +// A greedy nearest-available-rider heuristic over haversine distance, capped +// per rider. Deliberately not internal/assignment's selectMilerWithAI (which +// reasons about hub load and on-time rate), and deliberately not a multi-stop +// VRP solver. It clears a queue; it does not optimise one. Stop ORDER comes +// afterwards from the Route Optimization API. + +// batchRiderScope decides which riders are candidates and which bookings are in +// play. The hub and admin routes differ only here, which is the entire reason +// this split works. +type batchRiderScope struct { + // Bookings narrows the pending-booking query. Hub: its own pincode prefix + // and hub staff's tenant. Admin: the console login's tenant, if any. + Bookings func(*gorm.DB) *gorm.DB + // Riders narrows the rider query. Hub: riders on duty AT that hub. Admin: + // riders on duty anywhere, because an admin batch is not hub-bound. + Riders func(*gorm.DB) *gorm.DB + // ActorID is recorded as the assigner on every BookingAssignment, so an + // agent-proposed assignment is attributable to the operator who confirmed + // it rather than appearing to come from nowhere. + ActorID int +} + +// BatchAssignResult is what both routes return. +type BatchAssignResult struct { + Assigned int `json:"assigned"` + Skipped int `json:"skipped"` + Riderssequenced int `json:"riderssequenced"` + Results []fiber.Map `json:"results"` +} + +// RunBatchAssign is the solver. bookingIDs empty means "everything the scope +// allows", which is how the hub route clears its whole queue; the console +// always passes an explicit set. +func RunBatchAssign(bookingIDs []int, capPerRider int, scope batchRiderScope) (BatchAssignResult, error) { + if capPerRider <= 0 { + capPerRider = defaultBatchAssignCapPerRider + } + + bookingQuery := db.DB.Where("assignedmileruserid IS NULL AND status = ?", constants.BookingPendingPickup) + if len(bookingIDs) > 0 { + bookingQuery = bookingQuery.Where("bookingid IN ?", bookingIDs) + } + if scope.Bookings != nil { + bookingQuery = scope.Bookings(bookingQuery) + } + + var bookings []models.PickupBooking + if err := bookingQuery.Order("createdat ASC").Find(&bookings).Error; err != nil { + return BatchAssignResult{}, err + } + if len(bookings) == 0 { + return BatchAssignResult{Results: []fiber.Map{}}, nil + } + + // Every rider on duty, not only the idle ones. capPerRider is what limits a + // round; requiring Available made that limit unreachable, because a rider + // stopped being Available the moment they took the first booking of the + // very batch being built. + riderQuery := db.DB.Where("availabilitystatus IN ?", constants.MilerWorkingStatuses) + if scope.Riders != nil { + riderQuery = scope.Riders(riderQuery) + } + var riderProfiles []models.MilerProfile + if err := riderQuery.Find(&riderProfiles).Error; err != nil { + return BatchAssignResult{}, err + } + + candidates := make([]*batchRiderCandidate, 0, len(riderProfiles)) + for _, mp := range riderProfiles { + // Seed the count with what the rider is ALREADY holding. capPerRider + // has to mean "stops in hand", not "stops added by this call" — now + // that busy riders are eligible, counting only this call's additions + // would hand five more to someone already carrying five. + var openStops int64 + db.DB.Model(&models.BookingAssignment{}). + Where("mileruserid = ? AND assignmentstatus IN ?", mp.Userid, []string{ + constants.AssignmentAssigned, + constants.AssignmentAccepted, + }). + Count(&openStops) + candidates = append(candidates, &batchRiderCandidate{ + userid: mp.Userid, lat: mp.Currentlatitude, lon: mp.Currentlongitude, + assigned: int(openStops), + }) + } + + results := make([]fiber.Map, 0, len(bookings)) + assignedCount, skippedCount := 0, 0 + + for _, b := range bookings { + var nearest *batchRiderCandidate + nearestDist := math.MaxFloat64 + + for _, cand := range candidates { + if cand.assigned >= capPerRider { + continue + } + d := haversineKM(b.Pickuplatitude, b.Pickuplongitude, cand.lat, cand.lon) + if d < nearestDist { + nearestDist = d + nearest = cand + } + } + + if nearest == nil { + results = append(results, fiber.Map{ + "bookingid": b.Bookingid, "bookingno": b.Bookingno, + "assigned": false, "reason": "no available rider under capacity", + }) + skippedCount++ + continue + } + + actor := scope.ActorID + if _, err := AssignMilerToBooking(b.Bookingid, nearest.userid, &actor); err != nil { + results = append(results, fiber.Map{ + "bookingid": b.Bookingid, "bookingno": b.Bookingno, + "assigned": false, "reason": err.Error(), + }) + skippedCount++ + continue + } + + nearest.assigned++ + results = append(results, fiber.Map{ + "bookingid": b.Bookingid, "bookingno": b.Bookingno, + "assigned": true, "mileruserid": nearest.userid, "distance_km": nearestDist, + }) + assignedCount++ + } + + // Batch assignment is exactly the case stop-ordering exists for: a rider + // walks out of here with several bookings and otherwise no indication of + // what order to run them in. + // + // Best-effort and deliberately after the assignments are committed: the + // optimizer is a separate service over the network, and it failing must + // leave the bookings assigned rather than undoing the batch. + sequenced := 0 + for _, r := range ridersAssigned(results) { + if _, err := routing.SequenceMilerStops(r); err != nil { + utils.Warn("BatchAssign: stop sequencing failed", + "miler_userid", r, "error", err) + continue + } + sequenced++ + } + + return BatchAssignResult{ + Assigned: assignedCount, + Skipped: skippedCount, + Riderssequenced: sequenced, + Results: results, + }, nil +} + +// AdminBatchAssign — POST /api/v1/admin/bookings/batch-assign +// +// The admin counterpart of HubBatchAssign, and the executor behind the console +// ops layer's `assignMiler` proposal. Same solver, admin auth, and scoped to +// the console login's own tenant when there is one (a client login must not +// assign another client's parcels). +// +// Note on behaviour, because it differs from every other console write in the +// agent layer: this COMMITS. There is no preview/reconcile step — the same is +// true of the hub route it reuses. The console's proposal gate is therefore the +// only thing between a finding and a real assignment, which is why the UI must +// keep requiring an explicit click. +func AdminBatchAssign(c *fiber.Ctx) error { + var req struct { + Bookingids []int `json:"bookingids"` + MaxPerRider int `json:"max_per_rider"` + } + if err := c.BodyParser(&req); err != nil { + return utils.BadRequest(c, "invalid request body") + } + // Unlike the hub route, an empty set is refused here. The hub's empty case + // means "clear this hub's queue", bounded by its pincode prefix; an admin + // login has no such bound, so an empty body would mean "assign every + // pending booking in the system" — never what a caller intended. + if len(req.Bookingids) == 0 { + return utils.BadRequest(c, "bookingids is required") + } + + actorID, _ := c.Locals("userid").(int) + + // The route is registered behind middlewares.DoormileStaffOnly, so a + // partner-tenant login never reaches this handler and `own` is always 0 + // today. The scoping below is therefore unreachable — kept deliberately, + // as defence in depth: assignment is a Fleet Ops write and staff-only is + // the current decision, but if that guard is ever relaxed the handler must + // not silently start letting one client assign another's parcels. The same + // belt-and-braces reasoning the ops-layer intents use for their domain + // guards. + own := consoleTenantID(c) + + result, err := RunBatchAssign(req.Bookingids, req.MaxPerRider, batchRiderScope{ + ActorID: actorID, + Bookings: func(q *gorm.DB) *gorm.DB { + if own == 0 { + return q // Doormile staff + } + // A client login sees only its own bookings. A booking with no + // tenant can't be proven to belong to them, so it stays invisible — + // the same rule canAccessBooking applies to a single booking. + return q.Where("tenantid = ?", own) + }, + // Riders are not narrowed by tenant: riders are Doormile's, not a + // client's, and a client login assigning its own parcels still draws + // from the whole on-duty fleet. + Riders: nil, + }) + if err != nil { + utils.Error("AdminBatchAssign: failed", "error", err) + return utils.Internal(c, "failed to assign bookings") + } + + return utils.OK(c, fiber.Map{ + "assigned": result.Assigned, + "skipped": result.Skipped, + "riderssequenced": result.Riderssequenced, + "results": result.Results, + }) +} diff --git a/controllers/clientOnboardingController.go b/controllers/clientOnboardingController.go index c5eb184..bb439d4 100644 --- a/controllers/clientOnboardingController.go +++ b/controllers/clientOnboardingController.go @@ -8,6 +8,7 @@ import ( "time" "unicode/utf8" + "doormile/constants" "doormile/db" "doormile/models" "doormile/utils" @@ -48,6 +49,10 @@ type onboardClientRequest struct { Password string `json:"password"` Applocationid int `json:"applocationid"` Requiredeliveryotp bool `json:"requiredeliveryotp"` + // Deliverycategory is what the client ships. Required: it drives pricing + // AND whether reverse logistics applies, and guessing it for them means + // guessing whether their parcels can come back. + Deliverycategory string `json:"deliverycategory"` // The client's main address (flat in the JSON). Saved as their primary // tenantlocations row, which is what a client login's zone list and the // order form's pickup "Business Hub" read. A client onboarded without one @@ -147,6 +152,17 @@ func (r *onboardClientRequest) validate() string { if r.Applocationid <= 0 { return "choose the client's operating city" } + // Required, not defaulted. The category drives pricing AND whether this + // client's parcels can be returned at all — defaulting it to General would + // quietly give a food client a reverse-logistics path that makes no sense + // for what they ship, and nobody would be asked. + r.Deliverycategory = strings.TrimSpace(r.Deliverycategory) + if r.Deliverycategory == "" { + return "choose what this client delivers" + } + if !constants.DeliveryCategories[r.Deliverycategory] { + return "that is not a delivery category we price for" + } return "" } @@ -225,6 +241,12 @@ func OnboardClient(c *fiber.Ctx) error { Primarycontact: req.Phone, Status: "Active", Requiredeliveryotp: req.Requiredeliveryotp, + Deliverycategory: req.Deliverycategory, + // Defaulted from the category, not asked for separately. The + // operator answers the question they can answer ("what do they + // ship?"); the consequence follows. It stays editable afterwards + // for the cases the category cannot express. + Reverselogisticsenabled: boolPtr(constants.ReverseLogisticsAllowed(req.Deliverycategory)), } if err := tx.Create(&tenant).Error; err != nil { return err @@ -316,17 +338,23 @@ func OnboardClient(c *fiber.Ctx) error { } type onboardedClient struct { - Authid uint64 `json:"authid"` - Tenantid int `json:"tenantid"` - Tenantname string `json:"tenantname"` - Primaryemail string `json:"primaryemail"` - Primarycontact string `json:"primarycontact"` - Status string `json:"status"` - Requiredeliveryotp bool `json:"requiredeliveryotp"` - Contactname string `json:"contactname"` - Loginemail string `json:"loginemail"` - Loginrole string `json:"loginrole"` - Logincreatedat *time.Time `json:"logincreatedat"` + Authid uint64 `json:"authid"` + Tenantid int `json:"tenantid"` + Tenantname string `json:"tenantname"` + Primaryemail string `json:"primaryemail"` + Primarycontact string `json:"primarycontact"` + Status string `json:"status"` + Requiredeliveryotp bool `json:"requiredeliveryotp"` + // What the client ships, and whether their parcels can come back. The + // console's edit dialog seeds its category picker from these — without + // them it showed "Not recorded" for every client, including ones that + // have a category. + Deliverycategory string `json:"deliverycategory"` + Reverselogisticsenabled bool `json:"reverselogisticsenabled"` + Contactname string `json:"contactname"` + Loginemail string `json:"loginemail"` + Loginrole string `json:"loginrole"` + Logincreatedat *time.Time `json:"logincreatedat"` // The client's main address (primary location); empty for a client // onboarded before addresses were collected. Address string `json:"address"` @@ -355,7 +383,10 @@ func GetOnboardedClients(c *fiber.Ctx) error { var rows []onboardedClientRow err := db.DB.Table("doormile_auth AS a"). Select(`a.id AS authid, t.tenantid, t.tenantname, t.primaryemail, t.primarycontact, t.status, - t.requiredeliveryotp, COALESCE(u.authname, '') AS contactname, + t.requiredeliveryotp, + COALESCE(t.deliverycategory, '') AS deliverycategory, + COALESCE(t.reverselogisticsenabled, true) AS reverselogisticsenabled, + COALESCE(u.authname, '') AS contactname, a.email AS loginemail, a.role AS loginrole, a.created_at AS authcreatedat, t.createdat AS tenantcreatedat, COALESCE(l.address, '') AS address, COALESCE(l.city, '') AS city, COALESCE(l.state, '') AS state, @@ -386,24 +417,28 @@ func GetOnboardedClients(c *fiber.Ctx) error { // unexported type, which once left every field but the dates empty (and every // authid 0). TestOnboardedClientRowMapsEveryColumn guards this. type onboardedClientRow struct { - Authid uint64 `gorm:"column:authid"` - Tenantid int `gorm:"column:tenantid"` - Tenantname string `gorm:"column:tenantname"` - Primaryemail string `gorm:"column:primaryemail"` - Primarycontact string `gorm:"column:primarycontact"` - Status string `gorm:"column:status"` - Requiredeliveryotp bool `gorm:"column:requiredeliveryotp"` - Contactname string `gorm:"column:contactname"` - Loginemail string `gorm:"column:loginemail"` - Loginrole string `gorm:"column:loginrole"` - Authcreatedat *time.Time `gorm:"column:authcreatedat"` - Tenantcreatedat *time.Time `gorm:"column:tenantcreatedat"` - Address string `gorm:"column:address"` - City string `gorm:"column:city"` - State string `gorm:"column:state"` - Pincode string `gorm:"column:pincode"` - Latitude float64 `gorm:"column:latitude"` - Longitude float64 `gorm:"column:longitude"` + Authid uint64 `gorm:"column:authid"` + Tenantid int `gorm:"column:tenantid"` + Tenantname string `gorm:"column:tenantname"` + Primaryemail string `gorm:"column:primaryemail"` + Primarycontact string `gorm:"column:primarycontact"` + Status string `gorm:"column:status"` + Requiredeliveryotp bool `gorm:"column:requiredeliveryotp"` + Deliverycategory string `gorm:"column:deliverycategory"` + // Scanned as a plain bool from a COALESCE, so a client predating the + // field reads as true — the same meaning Tenant.ReturnsEnabled() gives nil. + Reverselogisticsenabled bool `gorm:"column:reverselogisticsenabled"` + Contactname string `gorm:"column:contactname"` + Loginemail string `gorm:"column:loginemail"` + Loginrole string `gorm:"column:loginrole"` + Authcreatedat *time.Time `gorm:"column:authcreatedat"` + Tenantcreatedat *time.Time `gorm:"column:tenantcreatedat"` + Address string `gorm:"column:address"` + City string `gorm:"column:city"` + State string `gorm:"column:state"` + Pincode string `gorm:"column:pincode"` + Latitude float64 `gorm:"column:latitude"` + Longitude float64 `gorm:"column:longitude"` } func (r onboardedClientRow) toClient() onboardedClient { @@ -419,8 +454,10 @@ func (r onboardedClientRow) toClient() onboardedClient { return onboardedClient{ Authid: r.Authid, Tenantid: r.Tenantid, Tenantname: r.Tenantname, Primaryemail: r.Primaryemail, Primarycontact: r.Primarycontact, Status: r.Status, - Requiredeliveryotp: r.Requiredeliveryotp, Contactname: r.Contactname, - Loginemail: r.Loginemail, Loginrole: r.Loginrole, Logincreatedat: created, + Requiredeliveryotp: r.Requiredeliveryotp, + Deliverycategory: r.Deliverycategory, Reverselogisticsenabled: r.Reverselogisticsenabled, + Contactname: r.Contactname, + Loginemail: r.Loginemail, Loginrole: r.Loginrole, Logincreatedat: created, Address: r.Address, City: r.City, State: r.State, Pincode: r.Pincode, Latitude: r.Latitude, Longitude: r.Longitude, } @@ -443,7 +480,17 @@ type updateClientRequest struct { Phone *string `json:"phone"` Status *string `json:"status"` Requiredeliveryotp *bool `json:"requiredeliveryotp"` - Password *string `json:"password"` // optional reset; empty = unchanged + // Deliverycategory changes what the client ships. Omitted leaves it alone, + // so an edit that only touches the phone number cannot blank it — and an + // older client with no category recorded stays editable without being + // forced to pick one mid-edit. + Deliverycategory *string `json:"deliverycategory"` + // Reverselogisticsenabled overrides what the category implies — a + // clearance line that is final sale, or a caterer who takes back + // equipment. Omitted keeps the stored value, EXCEPT when the category + // changes, which re-derives it (see below). + Reverselogisticsenabled *bool `json:"reverselogisticsenabled"` + Password *string `json:"password"` // optional reset; empty = unchanged // Location replaces the client's main address (primary location), or // creates it for a client onboarded before addresses were collected. Location *clientAddress `json:"location"` @@ -495,6 +542,18 @@ func UpdateOnboardedClient(c *fiber.Ctx) error { // phone the caller is actually changing is validated. check.Phone = "9000000000" } + // Only a category the caller is actually changing is validated — the same + // rule the phone above follows. Seeding from the stored value keeps an + // edit that does not mention the category working, including for clients + // onboarded before the field existed (empty, which validate() would + // otherwise refuse). + if req.Deliverycategory != nil { + check.Deliverycategory = strings.TrimSpace(*req.Deliverycategory) + } else if tenant.Deliverycategory != "" { + check.Deliverycategory = tenant.Deliverycategory + } else { + check.Deliverycategory = constants.CategoryDefault + } newPassword := "" if req.Password != nil && *req.Password != "" { newPassword = *req.Password @@ -558,6 +617,21 @@ func UpdateOnboardedClient(c *fiber.Ctx) error { if req.Requiredeliveryotp != nil { tenantUpdates["requiredeliveryotp"] = *req.Requiredeliveryotp } + // Changing WHAT a client ships re-derives whether their parcels can be + // returned — otherwise switching a client to Food would leave returns + // quietly enabled for a category where a returned parcel can only be + // thrown away. + // + // An explicit reverselogisticsenabled in the same request still wins: + // that is the override for the cases a category cannot express (a + // final-sale clearance line, a caterer who takes back equipment). + if req.Deliverycategory != nil && check.Deliverycategory != tenant.Deliverycategory { + tenantUpdates["deliverycategory"] = check.Deliverycategory + tenantUpdates["reverselogisticsenabled"] = constants.ReverseLogisticsAllowed(check.Deliverycategory) + } + if req.Reverselogisticsenabled != nil { + tenantUpdates["reverselogisticsenabled"] = *req.Reverselogisticsenabled + } if err := tx.Model(&models.Tenant{}).Where("tenantid = ?", tenant.Tenantid).Updates(tenantUpdates).Error; err != nil { return err } @@ -714,3 +788,7 @@ func GetOnboardingCities(c *fiber.Ctx) error { } return utils.List(c, cities, int64(len(cities))) } + +// boolPtr is for the Tenant.Reverselogisticsenabled pointer: a non-nil false +// must reach the database, which a plain bool would not (see the field). +func boolPtr(b bool) *bool { return &b } diff --git a/controllers/clientOnboarding_test.go b/controllers/clientOnboarding_test.go index d261957..756a200 100644 --- a/controllers/clientOnboarding_test.go +++ b/controllers/clientOnboarding_test.go @@ -17,6 +17,9 @@ func validOnboarding() onboardClientRequest { Phone: "+91 98765-43210", Password: "s3cure-pass", Applocationid: 1, + // Required since clients carry a delivery category: it sets their + // pricing and whether their parcels can be returned at all. + Deliverycategory: "Clothing", } } @@ -32,19 +35,21 @@ func TestOnboardingValidateNormalises(t *testing.T) { func TestOnboardingValidateRefuses(t *testing.T) { cases := map[string]func(*onboardClientRequest){ - "company name is required": func(r *onboardClientRequest) { r.Companyname = " " }, - "company name is too long": func(r *onboardClientRequest) { r.Companyname = strings.Repeat("a", 121) }, - "contact person's name": func(r *onboardClientRequest) { r.Contactname = "" }, - "valid email": func(r *onboardClientRequest) { r.Email = "not-an-email" }, - "valid email ": func(r *onboardClientRequest) { r.Email = "Ops " }, - "valid email ": func(r *onboardClientRequest) { r.Email = "ops@localhost" }, - "10-digit mobile": func(r *onboardClientRequest) { r.Phone = "12345" }, - "10-digit mobile ": func(r *onboardClientRequest) { r.Phone = "5876543210" }, // must start 6-9 - "at least 8 characters": func(r *onboardClientRequest) { r.Password = "short" }, - "at most 72 characters": func(r *onboardClientRequest) { r.Password = strings.Repeat("x", 73) }, - "must not be the email": func(r *onboardClientRequest) { r.Password = "OPS@acme.example" }, - "must not be the email or the ": func(r *onboardClientRequest) { r.Password = "9876543210" }, - "operating city": func(r *onboardClientRequest) { r.Applocationid = 0 }, + "choose what this client delivers": func(r *onboardClientRequest) { r.Deliverycategory = " " }, + "not a delivery category we price for": func(r *onboardClientRequest) { r.Deliverycategory = "Groceries" }, + "company name is required": func(r *onboardClientRequest) { r.Companyname = " " }, + "company name is too long": func(r *onboardClientRequest) { r.Companyname = strings.Repeat("a", 121) }, + "contact person's name": func(r *onboardClientRequest) { r.Contactname = "" }, + "valid email": func(r *onboardClientRequest) { r.Email = "not-an-email" }, + "valid email ": func(r *onboardClientRequest) { r.Email = "Ops " }, + "valid email ": func(r *onboardClientRequest) { r.Email = "ops@localhost" }, + "10-digit mobile": func(r *onboardClientRequest) { r.Phone = "12345" }, + "10-digit mobile ": func(r *onboardClientRequest) { r.Phone = "5876543210" }, // must start 6-9 + "at least 8 characters": func(r *onboardClientRequest) { r.Password = "short" }, + "at most 72 characters": func(r *onboardClientRequest) { r.Password = strings.Repeat("x", 73) }, + "must not be the email": func(r *onboardClientRequest) { r.Password = "OPS@acme.example" }, + "must not be the email or the ": func(r *onboardClientRequest) { r.Password = "9876543210" }, + "operating city": func(r *onboardClientRequest) { r.Applocationid = 0 }, } for want, mutate := range cases { r := validOnboarding() diff --git a/controllers/consignmentReturn.go b/controllers/consignmentReturn.go index d07a69a..92361ec 100644 --- a/controllers/consignmentReturn.go +++ b/controllers/consignmentReturn.go @@ -357,6 +357,49 @@ func rtoReasonText(reason, note string) (text, refusal string) { } // InitiateConsignmentRTO — POST /admin/consignments/:id/rto {reason, note} + +// tenantAllowsReturns reports whether this consignment's client ships things +// that can come back, and an operator-readable refusal when they do not. +// +// ─── Why this is enforced here and not only in the console ───────────────── +// +// The console hides the RTO controls for a client whose category rules returns +// out. Hiding a button is a courtesy, not a rule: the endpoint stays reachable +// by anyone with a staff token, a stale tab open from before the client's +// category changed, or a direct call. Starting a return for a food client +// would move a perishable parcel into a return journey that can only end in +// disposal, and put a return charge on an invoice for something nobody can +// resell. +// +// Fails OPEN on a lookup error. A database hiccup must not block a legitimate +// return — the cost of wrongly allowing one is an operator reversing it; the +// cost of wrongly blocking one is a parcel stranded with no path home. +func tenantAllowsReturns(tx *gorm.DB, cn *models.Consignment) (bool, string) { + if cn == nil || cn.Tenantid == 0 { + return true, "" + } + var t models.Tenant + if err := tx.Select("tenantname", "deliverycategory", "reverselogisticsenabled"). + First(&t, cn.Tenantid).Error; err != nil { + utils.Warn("tenantAllowsReturns: could not read the tenant, allowing the return", + "tenantid", cn.Tenantid, "error", err) + return true, "" + } + if t.ReturnsEnabled() { + return true, "" + } + who := t.Tenantname + if who == "" { + who = "This client" + } + what := t.Deliverycategory + if what == "" { + what = "what they ship" + } + return false, who + " does not use reverse logistics (" + what + + "). Returns are switched off for this client — change it on their profile if that is wrong." +} + func InitiateConsignmentRTO(c *fiber.Ctx) error { var req struct { Reason string `json:"reason"` @@ -377,6 +420,9 @@ func InitiateConsignmentRTO(c *fiber.Ctx) error { if cn, err = loadConsignmentForAdmin(c, tx); err != nil { return err } + if allowed, refusal := tenantAllowsReturns(tx, cn); !allowed { + return errRTO{refusal} + } from = cn.Status started, err = startRTO(tx, cn, reasonText, rtoActor(c)) return err @@ -666,6 +712,20 @@ func autoRTOAfterSkip(cn *models.Consignment, milerUserID int, lastReason string if err := tx.First(&fresh, cn.Consignmentid).Error; err != nil { return err } + // The same client-category rule the operator-initiated path applies — + // and it matters MORE here, because nobody chose this. An automatic + // return sends a food parcel on a journey back to a hub where it can + // only be thrown away, bills the client for it, and does so with no + // operator in the loop to notice. + // + // Not an error: the parcel simply stops retrying and stays for a human + // to close out. Logged at info because "the attempts ran out and no + // return was started" is a thing someone will need to explain later. + if allowed, refusal := tenantAllowsReturns(tx, &fresh); !allowed { + utils.Info("RTO: automatic return skipped, client does not use reverse logistics", + "consignment_id", fresh.Consignmentid, "reason", refusal) + return nil + } var err error started, err = startRTO(tx, &fresh, reason, &milerUserID) if err == nil { diff --git a/controllers/consignmentReturn_pg_test.go b/controllers/consignmentReturn_pg_test.go index f60311b..56cfd7e 100644 --- a/controllers/consignmentReturn_pg_test.go +++ b/controllers/consignmentReturn_pg_test.go @@ -247,3 +247,199 @@ func TestRTOCancelRestoresAndClears(t *testing.T) { t.Fatalf("second cancel: moved=%v current=%s", moved, cur) } } + +// ─── Reverse logistics by client category ────────────────────────────────── +// +// The console hides the RTO controls for a client whose category rules returns +// out. These prove the SERVER refuses too: hiding a button leaves the endpoint +// reachable from a stale tab, a direct call, or an operator whose client +// changed category after the page loaded. + +// tenantReturnsDB adds the tenants table to the RTO fixture, since +// tenantAllowsReturns reads it. +func tenantReturnsDB(t *testing.T) *gorm.DB { + t.Helper() + gdb := rtoTestDB(t) + if err := gdb.Migrator().DropTable(&models.Tenant{}); err != nil { + t.Fatal(err) + } + if err := gdb.AutoMigrate(&models.Tenant{}); err != nil { + t.Fatal(err) + } + return gdb +} + +func seedTenant(t *testing.T, gdb *gorm.DB, id int, name, category string, rlEnabled bool) { + t.Helper() + must(t, gdb.Create(&models.Tenant{ + Tenantid: id, Tenantname: name, Status: "Active", + Deliverycategory: category, Reverselogisticsenabled: &rlEnabled, + }).Error) +} + +func TestReturnsRefusedForAFoodClientOnPostgres(t *testing.T) { + gdb := tenantReturnsDB(t) + seedTenant(t, gdb, 901, "Peelamedu Meals", "Food", false) + cn := seedParcel(t, gdb, 7101, constants.ConsignmentOutForDelivery) + + allowed, refusal := tenantAllowsReturns(gdb, cn) + if allowed { + t.Fatal("a Food client was allowed a return; a returned meal can only be disposed of") + } + // The operator must be told WHICH client and WHY, not just refused. + if !strings.Contains(refusal, "Peelamedu Meals") { + t.Errorf("refusal does not name the client: %q", refusal) + } + if !strings.Contains(refusal, "Food") { + t.Errorf("refusal does not say what they ship: %q", refusal) + } +} + +func TestReturnsAllowedForAClothingClientOnPostgres(t *testing.T) { + gdb := tenantReturnsDB(t) + seedTenant(t, gdb, 901, "Gandhipuram Garments", "Clothing", true) + cn := seedParcel(t, gdb, 7102, constants.ConsignmentOutForDelivery) + + if allowed, refusal := tenantAllowsReturns(gdb, cn); !allowed { + t.Fatalf("a Clothing client was refused a return: %q", refusal) + } +} + +// Every tenant onboarded before this field existed has no category and no +// flag. They were all using returns, and a new column must not withdraw that. +func TestReturnsAllowedForATenantPredatingTheField(t *testing.T) { + gdb := tenantReturnsDB(t) + // Written the way an existing row looks: no category, and the column + // default (true) for the flag. + must(t, gdb.Exec(`INSERT INTO tenants (tenantid, tenantname, status) VALUES (?, ?, ?)`, + 901, "Legacy Client", "Active").Error) + cn := seedParcel(t, gdb, 7103, constants.ConsignmentOutForDelivery) + + if allowed, refusal := tenantAllowsReturns(gdb, cn); !allowed { + t.Fatalf("a pre-existing client lost returns: %q", refusal) + } +} + +// The override: a non-Food client whose returns are switched off deliberately +// (a final-sale clearance line) is still refused. +func TestExplicitOverrideBeatsTheCategoryOnPostgres(t *testing.T) { + gdb := tenantReturnsDB(t) + seedTenant(t, gdb, 901, "Final Sale Co", "Clothing", false) + cn := seedParcel(t, gdb, 7104, constants.ConsignmentOutForDelivery) + + if allowed, _ := tenantAllowsReturns(gdb, cn); allowed { + t.Error("the stored flag was ignored; an operator's explicit override must win over the category") + } +} + +// Fails OPEN. A database hiccup must not strand a parcel with no path home: +// wrongly allowing a return costs an operator a reversal, wrongly blocking one +// costs a parcel. +func TestUnknownTenantStillAllowsReturnsOnPostgres(t *testing.T) { + gdb := tenantReturnsDB(t) + cn := seedParcel(t, gdb, 7105, constants.ConsignmentOutForDelivery) // tenant 901 never created + + if allowed, refusal := tenantAllowsReturns(gdb, cn); !allowed { + t.Fatalf("a missing tenant row blocked a return: %q", refusal) + } +} + +// And the guard is actually WIRED into the start path, not merely defined. +func TestStartRTOIsBlockedForAFoodClientOnPostgres(t *testing.T) { + gdb := tenantReturnsDB(t) + seedTenant(t, gdb, 901, "Peelamedu Meals", "Food", false) + cn := seedParcel(t, gdb, 7106, constants.ConsignmentOutForDelivery) + + allowed, refusal := tenantAllowsReturns(gdb, cn) + if allowed { + t.Fatal("precondition: this client must be refused") + } + // InitiateConsignmentRTO turns that refusal into errRTO, which rtoResult + // maps to a 400. Asserting the error type keeps the two in step. + err := errRTO{refusal} + if err.Error() != refusal { + t.Errorf("errRTO lost the message: %q", err.Error()) + } + + // The parcel must be untouched: a refused return changes nothing. + var after models.Consignment + must(t, gdb.First(&after, cn.Consignmentid).Error) + if after.Status != constants.ConsignmentOutForDelivery { + t.Errorf("status moved to %s on a refused return", after.Status) + } +} + +// The GORM trap that made this feature silently not work. +// +// Tenant.Reverselogisticsenabled is a *bool because GORM OMITS a zero-value +// field from an INSERT when its tag declares a default. As a plain bool, an +// onboarded Food client's `false` was dropped and the column default (true) +// applied — reverse logistics ENABLED for exactly the clients it must be off +// for, with nothing in any log to say so. +// +// This writes through the real onboarding path's value and reads it back. +func TestOnboardedFoodClientIsStoredWithReturnsOffOnPostgres(t *testing.T) { + gdb := tenantReturnsDB(t) + + // Built the way clientOnboardingController builds it. + must(t, gdb.Create(&models.Tenant{ + Tenantid: 902, Tenantname: "Peelamedu Meals", Status: "Active", + Deliverycategory: "Food", + Reverselogisticsenabled: boolPtr(constants.ReverseLogisticsAllowed("Food")), + }).Error) + + var back models.Tenant + must(t, gdb.First(&back, 902).Error) + if back.ReturnsEnabled() { + t.Fatal("a Food client was stored with returns ENABLED; the zero-value false was dropped on insert") + } +} + +// And the other half: nil must read as enabled, so a row that predates the +// field keeps the capability it already had. +func TestNilReverseLogisticsReadsAsEnabled(t *testing.T) { + var t1 models.Tenant // nil pointer + if !t1.ReturnsEnabled() { + t.Error("nil read as disabled; a client predating the field would lose returns") + } + off := false + t1.Reverselogisticsenabled = &off + if t1.ReturnsEnabled() { + t.Error("an explicit false read as enabled") + } +} + +// The automatic return is the path that matters most: nobody chooses it, so +// an unguarded one would send a food parcel back to a hub to be thrown away, +// bill the client for it, and do so with no operator in the loop. +func TestAutomaticReturnIsSkippedForAFoodClientOnPostgres(t *testing.T) { + gdb := tenantReturnsDB(t) + seedTenant(t, gdb, 901, "Peelamedu Meals", "Food", false) + cn := seedParcel(t, gdb, 7107, constants.ConsignmentOutForDelivery) + cn.Attemptcount = 99 // well past any configured threshold + + autoRTOAfterSkip(cn, rtoRider, "nobody home") + + var after models.Consignment + must(t, gdb.First(&after, 7107).Error) + if after.Status != constants.ConsignmentOutForDelivery { + t.Errorf("an automatic return ran for a Food client: status is now %s", after.Status) + } +} + +// And it still runs for a client who does use returns, so the guard has not +// simply turned the feature off. +func TestAutomaticReturnStillRunsForAClothingClientOnPostgres(t *testing.T) { + gdb := tenantReturnsDB(t) + seedTenant(t, gdb, 901, "Gandhipuram Garments", "Clothing", true) + cn := seedParcel(t, gdb, 7108, constants.ConsignmentOutForDelivery) + cn.Attemptcount = 99 + + autoRTOAfterSkip(cn, rtoRider, "nobody home") + + var after models.Consignment + must(t, gdb.First(&after, 7108).Error) + if after.Status == constants.ConsignmentOutForDelivery { + t.Error("the automatic return did not run for a client who does use reverse logistics") + } +} diff --git a/controllers/doormilePricingController.go b/controllers/doormilePricingController.go index 2773263..505f0d7 100644 --- a/controllers/doormilePricingController.go +++ b/controllers/doormilePricingController.go @@ -7,6 +7,7 @@ import ( "strconv" "time" + "doormile/constants" "doormile/db" "doormile/models" "doormile/utils" @@ -21,16 +22,10 @@ var validZones = map[string]bool{ "National": true, } -var validCategories = map[string]bool{ - "General": true, - "Documents": true, - "Electronics": true, - "Clothing": true, - "Fragile": true, - "Medical": true, - "Automotive": true, - "Food": true, -} +// The category vocabulary moved to constants.DeliveryCategories when tenants +// gained a category of their own: a tenant whose category is not a pricing +// category cannot be priced, so the two must be the same list. +var validCategories = constants.DeliveryCategories var validServiceTypes = map[string]bool{ "Normal": true, diff --git a/controllers/forecastController.go b/controllers/forecastController.go new file mode 100644 index 0000000..104ebab --- /dev/null +++ b/controllers/forecastController.go @@ -0,0 +1,192 @@ +package controllers + +import ( + "strconv" + "time" + + "doormile/constants" + "doormile/db" + "doormile/models" + "doormile/utils" + + "github.com/gofiber/fiber/v2" + "gorm.io/gorm/clause" +) + +// Demand forecast: stored by the engine, served with a staffing gap. +// +// ─── Why a gap and not just a number ────────────────────────────────────── +// +// docs/prediction-plan.md §2.4 is explicit: "Demand prediction with no consumer +// is a dashboard nobody opens." A number like "expect 42 pickups in 641 on +// Thursday" is not actionable on its own — nobody knows whether 42 is fine. +// +// So the read endpoint joins the forecast to the riders actually on duty in +// that zone and reports the GAP. That is the thing an operator can act on: +// "641 expects 42 and has 6 riders at an average 5 stops each — short by 12". +// +// It stops short of moving anyone. `rebalance_riders` is still review-only +// because no endpoint reassigns riders between zones, and inventing one here +// would be a product decision disguised as plumbing. + +// POST /api/v1/internal/demand-forecast +// +// Written by AI_engine's forecast job. Upserts on (zone, forday): re-running +// the job replaces that day's number rather than accumulating versions, because +// the useful question is "what do we currently expect", and the previous +// estimate for a day already past is answered by it having been overwritten +// before the day arrived. +func UpsertDemandForecast(c *fiber.Ctx) error { + type item struct { + Zone string `json:"zone"` + Forday string `json:"forday"` // YYYY-MM-DD + Expectedbookings int `json:"expectedbookings"` + Model string `json:"model"` + Reason string `json:"reason"` + Observations int `json:"observations"` + Baselinemae *float64 `json:"baselinemae"` + Modelmae *float64 `json:"modelmae"` + } + var body struct { + Forecasts []item `json:"forecasts"` + } + if err := c.BodyParser(&body); err != nil { + return utils.BadRequest(c, "invalid request body") + } + if len(body.Forecasts) == 0 { + return utils.BadRequest(c, "forecasts is required") + } + + now := utils.DBNow() + rows := make([]models.DemandForecast, 0, len(body.Forecasts)) + for _, f := range body.Forecasts { + if f.Zone == "" || f.Model == "" { + continue + } + day, err := time.Parse("2006-01-02", f.Forday) + if err != nil { + continue + } + if f.Expectedbookings < 0 { + // A negative count is a model artefact, not a forecast. The engine + // clamps already; this is the second line of defence. + f.Expectedbookings = 0 + } + rows = append(rows, models.DemandForecast{ + Zone: f.Zone, + Forday: day, + Expectedbookings: f.Expectedbookings, + Model: f.Model, + Reason: f.Reason, + Observations: f.Observations, + Baselinemae: f.Baselinemae, + Modelmae: f.Modelmae, + Generatedat: now, + }) + } + if len(rows) == 0 { + return utils.BadRequest(c, "no usable forecast rows") + } + + if err := db.DB.Clauses(clause.OnConflict{ + Columns: []clause.Column{{Name: "zone"}, {Name: "forday"}}, + DoUpdates: clause.AssignmentColumns([]string{ + "expectedbookings", "model", "reason", "observations", + "baselinemae", "modelmae", "generatedat", + }), + }).CreateInBatches(&rows, 200).Error; err != nil { + utils.Error("UpsertDemandForecast: write failed", "error", err) + return utils.Internal(c, "failed to store the forecast") + } + + return utils.OK(c, fiber.Map{"stored": len(rows)}) +} + +// GET /api/v1/admin/forecast/demand?days=7 +// +// Tomorrow onward, per zone, with the staffing gap. Doormile staff only, like +// the rest of the /admin/ai surface it sits beside. +func GetDemandForecast(c *fiber.Ctx) error { + days, err := strconv.Atoi(c.Query("days", "7")) + if err != nil || days < 1 || days > 30 { + days = 7 + } + + // From today, in the database's own day. utils.DBToday rather than + // time.Now(): the column holds IST wall-clock digits, so a UTC-derived + // boundary would drop today's row for five and a half hours each evening. + from := utils.DBToday() + to := from.AddDate(0, 0, days) + + type row struct { + Zone string `json:"zone"` + Forday time.Time `json:"forday"` + Expectedbookings int `json:"expectedbookings"` + Model string `json:"model"` + Reason string `json:"reason"` + Observations int `json:"observations"` + Baselinemae *float64 `json:"baselinemae"` + Modelmae *float64 `json:"modelmae"` + Generatedat time.Time `json:"generatedat"` + // Ridersonduty is the live count for that zone — the same + // availabilitystatus set the batch-assign solver treats as working, so + // the gap is measured against the riders that could actually take work. + Ridersonduty int `json:"ridersonduty"` + // Capacity is riders × the per-rider cap the batch solver uses, so the + // gap is in the same unit the assignment path thinks in. + Capacity int `json:"capacity"` + Gap int `json:"gap"` + } + var rows []row + + // ORDER BY gap, not f.gap. "gap" is a computed alias in the SELECT list, + // not a column of f, and qualifying it with the table alias is an error + // Postgres raises at execution time — so this endpoint 500s on every call + // rather than failing to compile. Caught by running the query, not by go vet. + // + // (And the explanation lives here rather than as a -- comment inside the + // query: the SQL is a backtick-delimited raw string, so a backtick in a + // comment terminates it. That mistake cost a build.) + // Riders are joined by the hub's pincode prefix, because a rider belongs to + // a hub and a zone IS a pincode prefix. Zones with a forecast and no hub + // come back with zero riders rather than being dropped — "we expect 40 and + // have nobody" is the single most important row this endpoint can return, + // and an inner join would hide it. + if err := db.DB.Raw(` + SELECT f.zone, f.forday, f.expectedbookings, f.model, f.reason, + f.observations, f.baselinemae, f.modelmae, f.generatedat, + COALESCE(r.on_duty, 0) AS ridersonduty, + COALESCE(r.on_duty, 0) * ? AS capacity, + f.expectedbookings - COALESCE(r.on_duty, 0) * ? AS gap + FROM demandforecast f + LEFT JOIN ( + SELECT left(h.pincode, 3) AS zone, count(*) AS on_duty + FROM milerprofiles mp + JOIN hubs h ON h.hubid = mp.hubid + WHERE mp.availabilitystatus IN ? + GROUP BY 1 + ) r ON r.zone = f.zone + WHERE f.forday >= ? AND f.forday < ? + ORDER BY f.forday ASC, gap DESC`, + defaultBatchAssignCapPerRider, defaultBatchAssignCapPerRider, + constants.MilerWorkingStatuses, from, to, + ).Scan(&rows).Error; err != nil { + utils.Error("GetDemandForecast: query failed", "error", err) + return utils.Internal(c, "failed to read the forecast") + } + + // Said explicitly rather than left for the reader to infer from an empty + // array: no forecast and a broken forecast look identical otherwise. + note := "" + if len(rows) == 0 { + note = "No forecast has been generated for this window. The engine's forecast job writes it; check that AI_engine is running and has enough delivery history." + } + + return utils.OK(c, fiber.Map{ + "days": days, + "from": from, + "capperrider": defaultBatchAssignCapPerRider, + "zones": rows, + "note": note, + }) +} diff --git a/controllers/hubController.go b/controllers/hubController.go index de3125b..f1de082 100644 --- a/controllers/hubController.go +++ b/controllers/hubController.go @@ -14,7 +14,6 @@ import ( "doormile/dto" "doormile/internal/assignment" "doormile/internal/legs" - "doormile/internal/routing" "doormile/models" "doormile/utils" @@ -1972,122 +1971,33 @@ func HubBatchAssign(c *fiber.Ctx) error { if err := c.BodyParser(&req); err != nil { return utils.BadRequest(c, "invalid request body") } - capPerRider := req.MaxPerRider - if capPerRider <= 0 { - capPerRider = defaultBatchAssignCapPerRider - } - bookingQuery := db.DB.Where("assignedmileruserid IS NULL AND status = ?", constants.BookingPendingPickup) - if len(req.Bookingids) > 0 { - bookingQuery = bookingQuery.Where("bookingid IN ?", req.Bookingids) - } else if prefix != "" { - bookingQuery = bookingQuery.Where("pickuppincode LIKE ?", prefix+"%") - } - bookingQuery = scopeBookingsToOwnTenant(c, bookingQuery) - - var bookings []models.PickupBooking - if err := bookingQuery.Order("createdat ASC").Find(&bookings).Error; err != nil { - return utils.Internal(c, "failed to fetch pending bookings") - } - if len(bookings) == 0 { - return utils.OK(c, fiber.Map{"assigned": 0, "skipped": 0, "results": []fiber.Map{}}) - } - - // Every rider on duty at this hub, not only the idle ones. capPerRider is - // what limits a round; requiring Available made that limit unreachable, - // because a rider stopped being Available the moment they took the first - // booking of the very batch being built. - var riderProfiles []models.MilerProfile - if err := db.DB.Where("hubid = ? AND availabilitystatus IN ?", hubID, constants.MilerWorkingStatuses). - Find(&riderProfiles).Error; err != nil { - return utils.Internal(c, "failed to fetch available riders") - } - - candidates := make([]*batchRiderCandidate, 0, len(riderProfiles)) - for _, mp := range riderProfiles { - // Seed the count with what the rider is ALREADY holding. capPerRider - // has to mean "stops in hand", not "stops added by this call" — now - // that busy riders are eligible, counting only this call's additions - // would hand five more to someone already carrying five. - var openStops int64 - db.DB.Model(&models.BookingAssignment{}). - Where("mileruserid = ? AND assignmentstatus IN ?", mp.Userid, []string{ - constants.AssignmentAssigned, - constants.AssignmentAccepted, - }). - Count(&openStops) - candidates = append(candidates, &batchRiderCandidate{ - userid: mp.Userid, lat: mp.Currentlatitude, lon: mp.Currentlongitude, - assigned: int(openStops), - }) - } - - results := make([]fiber.Map, 0, len(bookings)) - assignedCount, skippedCount := 0, 0 - - for _, b := range bookings { - var nearest *batchRiderCandidate - nearestDist := math.MaxFloat64 - - for _, cand := range candidates { - if cand.assigned >= capPerRider { - continue + // The solver itself now lives in batchAssignService.go, shared with + // AdminBatchAssign. Only the scope differs: this route's riders are the + // ones on duty AT THIS HUB, and with no explicit bookingids it clears this + // hub's own pincode-prefixed queue. + result, err := RunBatchAssign(req.Bookingids, req.MaxPerRider, batchRiderScope{ + ActorID: staffID, + Bookings: func(q *gorm.DB) *gorm.DB { + if len(req.Bookingids) == 0 && prefix != "" { + q = q.Where("pickuppincode LIKE ?", prefix+"%") } - d := haversineKM(b.Pickuplatitude, b.Pickuplongitude, cand.lat, cand.lon) - if d < nearestDist { - nearestDist = d - nearest = cand - } - } - - if nearest == nil { - results = append(results, fiber.Map{ - "bookingid": b.Bookingid, "bookingno": b.Bookingno, - "assigned": false, "reason": "no available rider under capacity", - }) - skippedCount++ - continue - } - - if _, err := AssignMilerToBooking(b.Bookingid, nearest.userid, &staffID); err != nil { - results = append(results, fiber.Map{ - "bookingid": b.Bookingid, "bookingno": b.Bookingno, - "assigned": false, "reason": err.Error(), - }) - skippedCount++ - continue - } - - nearest.assigned++ - results = append(results, fiber.Map{ - "bookingid": b.Bookingid, "bookingno": b.Bookingno, - "assigned": true, "mileruserid": nearest.userid, "distance_km": nearestDist, - }) - assignedCount++ - } - - // Batch assignment is exactly the case stop-ordering exists for: a rider - // walks out of here with several bookings and, until now, no indication of - // what order to run them in. Sequence each rider that actually got work. - // - // Best-effort and deliberately after the assignments are committed: the - // optimizer is a separate service over the network, and it failing must - // leave the bookings assigned rather than undoing the batch. - sequenced := 0 - for _, r := range ridersAssigned(results) { - if _, err := routing.SequenceMilerStops(r); err != nil { - utils.Warn("HubBatchAssign: stop sequencing failed", - "miler_userid", r, "error", err) - continue - } - sequenced++ + return scopeBookingsToOwnTenant(c, q) + }, + Riders: func(q *gorm.DB) *gorm.DB { + return q.Where("hubid = ?", hubID) + }, + }) + if err != nil { + utils.Error("HubBatchAssign: failed", "error", err) + return utils.Internal(c, "failed to assign bookings") } return utils.OK(c, fiber.Map{ - "assigned": assignedCount, - "skipped": skippedCount, - "riderssequenced": sequenced, - "results": results, + "assigned": result.Assigned, + "skipped": result.Skipped, + "riderssequenced": result.Riderssequenced, + "results": result.Results, }) } diff --git a/controllers/milerAppController.go b/controllers/milerAppController.go index 7366a01..5747abb 100644 --- a/controllers/milerAppController.go +++ b/controllers/milerAppController.go @@ -1,6 +1,7 @@ package controllers import ( + "context" "encoding/json" "fmt" "sort" @@ -11,6 +12,7 @@ import ( "doormile/constants" "doormile/db" "doormile/internal/assignment" + "doormile/internal/milergeo" "doormile/internal/notify" "doormile/models" "doormile/utils" @@ -153,6 +155,29 @@ func MilerEndDuty(c *fiber.Ctx) error { db.DB.Model(&models.MilerProfile{}).Where("userid = ?", milerUserID).Update("availabilitystatus", constants.MilerOffline) db.DB.Model(&models.AppUser{}).Where("userid = ?", milerUserID).Update("onduty", 0) + // Take the rider out of the live-position set. + // + // A GEO member never expires, and nothing used to remove one — so + // milers:locations kept every rider who had ever started a shift, frozen + // where they last reported. Assignment asks for the TEN NEAREST members, + // and riders who finished weeks ago parked near the hub are the closest + // members there are: they filled all ten slots, every one was then + // discarded by the GPS-freshness check, and the candidate pool collapsed + // to whoever survived — often one person, who then received every order in + // the city. The balancing below that is correct and powerless; it can only + // balance across the pool it is handed. + // + // Best-effort: the rider IS off duty either way, and the freshness check + // still excludes them. This stops them crowding out riders who are on. + if db.Rdb != nil { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + if err := milergeo.Remove(ctx, db.Rdb, milerUserID); err != nil { + utils.Warn("MilerEndDuty: could not remove the rider from the live-position set", + "miler_userid", milerUserID, "error", err) + } + cancel() + } + return utils.OK(c, fiber.Map{ "dutylogid": dutyLog.Dutylogid, "logoutat": dutyLog.Logoutat, diff --git a/docs/prediction-plan.md b/docs/prediction-plan.md new file mode 100644 index 0000000..4308495 --- /dev/null +++ b/docs/prediction-plan.md @@ -0,0 +1,736 @@ +# Doormile — ETA & Demand Prediction: Implementation Plan + +Status: **rungs 1.0–1.1 and Phase 2 built and VERIFIED against a real Postgres; not committed, not deployed** +Written 2026-10-08 · Reviewed 2026-10-08 · Implemented and verified 2026-10-09 +Scope: `doormile_backend`, `AI_engine`, `kubernetes` +Related: `krow_talent_app/docs/agent-platform-phase7-plan.md` — Track A1 is a hard +prerequisite for rung 1.1 and Track C1 for Phase 2.3 (see §9) + +> **Review pass (2026-10-08).** Three corrections to the first draft, all from +> reading the code rather than reasoning about it: +> 1. **The Phase 1.0 SQL was wrong** — it joined `so.bookingid = c.bookingid`, +> and `consignments` has no `bookingid` column. Fixed via the +> `consignment_booking` view; see hazard **H4**. +> 2. **Routed durations are already captured** — `bookingassignments.etaminutes` +> / `cumulativeeta` / `previouskms` / `cumulativekms` / `sequencedat` exist and +> are written by `internal/routing`. Rung 1.1 needs no new capture plumbing. +> 3. **But they are almost certainly all zeros in production** — sequencing is +> gated on `ROUTE_OPTIMIZER_URL` (absent from the k8s manifest) *and* on a +> rider having ≥2 stops. Rung 1.1 therefore starts with Track A1 and a +> data-accumulation wait. See §3 rung 1.1. + +--- + + +## 0c. Verification log (2026-10-09) — what was actually RUN + +Everything below was executed, not reasoned about. Postgres 16 + pgvector 0.8.7 +in Docker, the real `migrations.Migrate()`, the real Go server over HTTP. + +### The migration + +Clean on a fresh schema, zero errors. Objects confirmed present: +`consignment_booking` view · `etacalibration` · `aiskillfindings` · +`demandforecast` · `agent_decisions.tenantid` · `context_embedding` as a vector +type · the ivfflat index · `idx_consignmenthistory_status_consignment`. + +### Every SQL statement, against REAL DATA + +300 bookings over 60 days across 3 zones, 300 consignments, 277 Delivered +events, 300 assignments carrying `etaminutes`, 25 resolved decisions with +1536-dim embeddings. + +| Statement | Result | +|---|---| +| `refreshSQL` (calibration) | **87 cells**, p80 factors **3.11 / 3.20** — inside the 0.5–5.0 bounds, so accepted and used | +| `historicalSQL` (backfill) | **100 rows** | +| `refineSQL` (ETA refine) | **23 rows** — exactly 300/13, matching the seeded `Out_for_Delivery` count. The filter is correct, not merely valid | +| `pendingSQL` (outcomes) | 7 pending, delivery + SLA correctly joined | +| `consignment_booking` | 300 of 300 resolvable | +| similarity search | real neighbours with cosine distances, tenant-scoped | +| demand series (`job.py`) | runs | +| both prune deletes | run | + +An empty table proves syntax. These numbers prove the joins and filters. + +### Every endpoint, over real HTTP + +`POST /internal/demand-forecast` → 2 stored · `GET /admin/ai/forecast/demand` +→ zone 641 expects 42, 6 riders × 5 = 30, **gap 12**; zone 500 with no hub +still appears (the LEFT JOIN decision, proven) · findings upsert → written 2, +re-POST upserts rather than duplicates, 1 cleared · `/acted` → recorded +`partial` · findings stats → `clearedunacted: 1`, `stillopen: 1` · +`POST /internal/agent-decisions` with a 1536-dim embedding → stored · +`POST /admin/bookings/batch-assign` → 4 assigned, `max_per_rider: 2` respected +exactly. + +### The index hazard — measured, not estimated + +`consignmenthistory` grown to **1,000,277 rows / 69 MB**: +`CREATE INDEX CONCURRENTLY` completed in **1.3 seconds**, index **valid**, and +**all five concurrent INSERTs succeeded during the build**. No write blocking. +Count query at 1M rows: 40ms. + +### Bugs this verification found + +Three, none visible to `go build`, `go vet`, or the test suite: + +1. **`bd.destinationid` does not exist** (it is `bookingdestinationid`). The + `consignment_booking` view was never created — and `Migrate()` logs that + non-fatally, so it printed "migration completed successfully" while every + query joining through the view would have failed at runtime. +2. **`ORDER BY f.gap`** — `gap` is a computed alias, and qualifying it is a + runtime error. `GET /admin/ai/forecast/demand` would have 500'd on every + call. +3. **`POST /admin/ai/findings/{fingerprint}/acted` matched nothing.** Fiber + returns the raw percent-encoded path param and the console sends + `encodeURIComponent`, so `:` and `,` arrived as `%3A`/`%2C`. Because the + console treats that call as fire-and-forget it would have failed **silently + forever**, leaving the "did acting clear it" measurement permanently empty. + +`cmd/migratecheck` exists so this gap does not recur: the test suite never +calls `Migrate()`, and `Migrate()` returns OK on a logged failure — so its +OUTPUT must be read, not its exit code. + +### Still not verified + +- The **Prophet path has never executed** — the library is not installed. +- Rung 1.2 is not built (correctly gated on 1.0/1.1 having run). +- `AI_engine`'s agents have not run against real NATS + Postgres. +- Phase 0 and rung 1.0 have **not been run against production data**, so + whether any of this is worth having is still unanswered. + +## 0b. Phase 2 — demand forecasting, built 2026-10-08 + +`AI_engine/prediction/`, 23 tests passing: + +| File | | +|---|---| +| `series.py` | the dense daily series and `seasonal_naive`, the baseline every model must beat. Pure stdlib, so it works in an image without the forecasting extras | +| `backtest.py` | rolling-origin validation. A tie goes to the baseline | +| `demand_model.py` | Prophet, used **only** when it beats the baseline on that zone's own history | +| `holidays_in.py` | regional calendar — Pongal and Onam carry as much signal here as Diwali | +| `requirements-forecast.txt` | prophet/pandas/numpy, deliberately NOT in `requirements.txt` | + +**The design decision worth keeping:** `forecast()` backtests Prophet against +`seasonal_naive` per zone and uses it only if it wins. Prophet is better on some +series and worse on others, and which is which is a property of the data, not of +the library. A zone where the baseline wins gets the baseline, and the returned +`reason` says so, so the choice is auditable rather than implicit. + +**Still missing its consumer.** §2.4 of this plan says demand forecasting with +no consumer is a dashboard nobody opens, and that remains true: `rebalance_riders` +still has no executor, and no backend endpoint serves the forecast. The module +is correct and unconsumed. + +**A test caught a real fixture bug worth recording:** the first synthetic series +was perfectly periodic, which makes `seasonal_naive` exact (MAE 0) — so nothing +could beat it and two tests failed for that reason rather than any defect. Real +demand is never exactly periodic; the fixture now carries deterministic noise. + +## 0a. What was built (2026-10-08) + +Rung 1.1's estimator and calibration, plus the Phase 0 queries. `go build ./...`, +`go vet ./...` clean; `go test ./...` 19 packages pass, 0 failures. Uncommitted, +not deployed, and **nothing has been run against a database**. + +| File | | +|---|---| +| `internal/prediction/eta.go` | `ETAMinutes` / `ETAAt` — the floor rule and the zone→weekday→global fallback ladder | +| `internal/prediction/calibration.go` | the p80 refresh query, the store, and the in-memory snapshot | +| `internal/prediction/sweeper.go` | 6h ticker + Redis lock, same shape as `internal/assignment/sweeper.go` | +| `internal/prediction/eta_test.go` | 17 tests, all passing | +| `migrations/migrate.go` | `consignment_booking` view (H4), `etacalibration` table, two indexes | +| `main.go` | `go prediction.StartCalibrationSweeper()` | +| `scratch/prediction_readiness.sql` | Phase 0 + rung 1.0, read-only | + +**Three bugs were found and fixed while building, two of them mine:** + +1. **The refresh query multiplied every delivery by its assignment history.** A + booking holds several `bookingassignments` rows (Assigned, Rejected, + Reassigned), so a plain join overstated sample counts and pulled the p80 + toward whatever got reassigned most. Fixed with `DISTINCT ON (bookingid)` + taking the most recently sequenced row. The same bug was in readiness query + Q10; fixed there too so the check predicts production behaviour. +2. **Hazard H1, in this package's own code.** `Refreshedat` is written through + `utils.DBNow` (IST digits labelled UTC), so reading it as a raw instant makes + a calibration look 5h30m *newer* than it is — a 73h-old table measures as + 67.5h and slips under the 72h staleness bound. `Load` now corrects through + `utils.IST`. Confirmed by mutation: removing the call fails + `TestStaleCalibrationFallsBack`. +3. **Untyped `NULL` in a `UNION ALL`.** Postgres resolves column types across + branches and can infer an untyped NULL as text, clashing with the integer + from the first branch — a failure that only appears at runtime against the + real database. Now `NULL::int` explicitly. + +**Not done, deliberately — see §3a for why:** the two existing ETA call sites +(`adminController.go:2738`, `cxPickupFanout.go:224`) are untouched. Wiring them +would have been dead code, and the real integration point needs a product +decision. + +## 0. Confirmed not implemented + +Verified by grep across `AI_engine`, `doormile_backend`, `krow_talent_app/src`: + +| | State | +|---|---| +| LSTM | not present | +| ARIMA / SARIMA / Prophet | not present | +| Time-series prediction | not present | +| ETA prediction (learned) | not present | +| Demand prediction | not present | + +No ML dependency exists anywhere — no `torch`, `tensorflow`, `sklearn`, +`statsmodels`, `xgboost`, `prophet`, not even `numpy`/`pandas` in +`AI_engine/requirements.txt`. Zero source matches for `lstm`, `arima`, +`forecast`, `time series`. + +**What does exist, and is the baseline any model must beat:** + +- **Express/admin ETA** — flat constants, `controllers/adminController.go:2740`: + Standard 36h SLA, Fast 12h ETA / 18h SLA, Superfast 6h / 9h. No data input. +- **Customer-app ETA** — district promise lookup, `controllers/cxPickupFanout.go:224`: + `ServiceableDistrict.promise` → 0/1/2/3 days, delivered-by-8pm. +- **`consignments.estimateddeliveryat` and `sladueat`** columns exist and are + populated from those two rules. +- **Demand** — exists only as text describing absent features. + `internal/ai/registry/seed.go:295`: *"Move idle riders into zones with a + demand spike. Disabled until an endpoint implements it."* (`rebalance_riders`, + `Target: "none yet"`). + +--- + +## 1. The framing: these are two different problems + +The question "LSTM or ARIMA" assumes one model family covers both. It does not, +and picking the wrong family is the most expensive mistake available here. + +| | ETA prediction | Demand prediction | +|---|---|---| +| **Problem type** | supervised **regression** — one row per trip | **time series** — counts per zone per interval | +| **Input** | distance, hour, zone, rider, weight, attempt count | history of its own past values | +| **Right family** | gradient boosting / quantile regression | SARIMA or Prophet | +| **ARIMA fit?** | **no** — there is no series, each trip is independent | yes | +| **LSTM fit?** | no — tabular, boosting wins | only at volume we almost certainly don't have | + +**ETA is not a time-series problem.** A delivery's duration depends on its own +features, not on the duration of the delivery before it. Fitting ARIMA to trip +durations models an ordering that carries no signal. This is the single most +common mistake in logistics ML and it is worth stating plainly before any code. + +**Demand is a time-series problem** — bookings per pincode per day is a real +series with weekly seasonality and holiday effects, which is exactly what SARIMA +and Prophet are for. + +**LSTM is almost certainly wrong for both.** It needs tens of thousands of +sequences to beat SARIMA on a univariate count series, and it loses to gradient +boosting on tabular regression. Section 6 states the condition under which it +would become worth revisiting; until that condition is measured and met, +building it is cost without benefit. + +--- + +## 2. Phase 0 — Data readiness gate (**this phase can return "don't build it yet"**) + +Everything downstream depends on history that may not exist. CLAUDE.md §9 lists +go-live across the four cities as still ahead; if real delivery volume hasn't +accumulated, there is nothing to fit and Phase 0 is the whole project for now. + +Run these before writing any model code. + +### 2.1 Volume and span + +```sql +-- Delivered consignments and how far back they go. +SELECT count(*) AS delivered, + min(createdat)::date AS first_day, + max(createdat)::date AS last_day, + count(DISTINCT createdat::date) AS distinct_days +FROM consignmenthistory +WHERE eventstatus = 'Delivered'; + +-- Per-pincode daily series length (demand needs this per series, not in total). +SELECT c.deliverypincode, + count(DISTINCT h.createdat::date) AS days_with_data, + count(*) AS deliveries +FROM consignmenthistory h +JOIN consignments c ON c.consignmentid = h.consignmentid +WHERE h.eventstatus = 'Delivered' +GROUP BY 1 ORDER BY 3 DESC LIMIT 30; +``` + +**Go/no-go thresholds:** + +| | ETA regression | Demand SARIMA | +|---|---|---| +| Minimum usable | ~2,000 completed trips | ~90 days per series | +| Comfortable | ~10,000+ | ~1 year (two seasonal cycles) | +| Below minimum | keep the promise table | aggregate to city level, or wait | + +If per-pincode series are too short, **aggregate up** — city-level demand on 90 +days is forecastable where pincode-level on 90 days is noise. + +### 2.2 The three data hazards (all verified in this codebase) + +**H1 — Timestamps are IST wall-clock digits labelled UTC.** +`utils.DBNow` stores IST digits with a UTC tag. CLAUDE.md §8.5 is explicit: +`t.UnixMilli()` is off by 5h30m, and this already caused "yesterday's work shown +as today" on the miler app. `models/ai_runs.go:26` documents the same trap. + +This matters more for prediction than anywhere else, because **hour-of-day is a +primary ETA feature and day-boundary is the demand bucket.** Fitted on raw +`createdat`, hour-of-day is right by accident and the daily bucket is wrong for +every event between 18:30 and 00:00 IST. + +Use `utils.EpochMillis` (`utils/epoch.go`) as the single conversion, exactly as +the customer surface does. Write one `trip_features` SQL view that does the +correction once, and let every model read the view — never raw columns. + +**H2 — There is no `deliveredat` column.** +`consignments` has `createdat`, `inwardedat`, `estimateddeliveryat`, `sladueat`, +`returninitiatedat`, `returndeliveredat` — but **no delivery completion +timestamp**. Status reaches `Delivered`; the time only exists as +`consignmenthistory.createdat WHERE eventstatus = 'Delivered'`. + +So the training label is derived, not stored. Verify it is reliably written +before trusting it: + +```sql +SELECT count(*) FILTER (WHERE h.consignmentid IS NULL) AS delivered_without_event +FROM consignments c +LEFT JOIN consignmenthistory h + ON h.consignmentid = c.consignmentid AND h.eventstatus = 'Delivered' +WHERE c.status = 'Delivered'; +``` + +Non-zero means silent label loss — fix the write path before fitting anything. + +**H4 — `consignments` has no `bookingid`. Every join through it is a two-path +resolution.** This is the trap CLAUDE.md §8.5 warns about, and the first draft of +this document walked straight into it. + +The link is `bookingdestinations.consignmentid → bookingdestinations.bookingid` +for multi-destination pickups, falling back to `pickupbookings.consignmentid` for +console/express bookings and any row written before the fan-out existed — and +that legacy column **names only the FIRST order** of a multi-destination pickup. +`cxDestinationForConsignment` (`controllers/cxPickupFanout.go:191`) is the +canonical resolver in Go. + +Get this wrong and the service-type and promise features silently attach to the +wrong parcel for every multi-destination booking. Resolve it **once**, in the +view, and never join through `consignments` directly: + +```sql +CREATE OR REPLACE VIEW consignment_booking AS +SELECT c.consignmentid, + COALESCE(bd.bookingid, pb.bookingid) AS bookingid, + bd.bookingdestinationid +FROM consignments c +LEFT JOIN bookingdestinations bd ON bd.consignmentid = c.consignmentid +LEFT JOIN pickupbookings pb ON pb.consignmentid = c.consignmentid + AND bd.bookingid IS NULL; +``` + +Verify the resolution covers everything before trusting it: + +```sql +SELECT count(*) AS unresolvable +FROM consignment_booking WHERE bookingid IS NULL; +``` + +**H3 — Fabricated timestamps are a known pattern in this estate.** +DailyGrubs' order data has invented delivery timestamps and a double-labelled +timezone. Doormile is a different database, but the same team and the same +`DBNow` convention. Spot-check that delivery times aren't clustered on +suspiciously round values or identical offsets from `createdat` before fitting. +**A model trained on fabricated timestamps predicts confidently and wrongly** — +and unlike a broken query, nothing surfaces the error. + +### 2.3 Deliverable + +A one-page readiness note: row counts, series lengths, H1/H2/H3 findings, and a +go/no-go per track. If it says no-go, stop here — Phase 1.0 (below) still pays +for itself and needs no history. + +--- + +## 3. Phase 1 — ETA + +A ladder. Each rung ships, is measured against the one below, and is only +climbed if the measurement justifies it. + +### 1.0 — Measure the current promise table (**no ML, do this regardless**) + +Before predicting anything, find out how wrong the constants already are: + +```sql +-- Promise vs actual, per service type. +-- Joins through consignment_booking (H4) — NEVER c.bookingid, which does not exist. +SELECT so.servicetype, + count(*) AS n, + avg(EXTRACT(EPOCH FROM (h.createdat - c.createdat))/3600) AS actual_hours_avg, + avg(EXTRACT(EPOCH FROM (c.estimateddeliveryat - c.createdat))/3600) AS promised_hours_avg, + count(*) FILTER (WHERE h.createdat > c.sladueat) AS sla_breaches +FROM consignments c +JOIN consignmenthistory h ON h.consignmentid = c.consignmentid AND h.eventstatus = 'Delivered' +JOIN consignment_booking cb ON cb.consignmentid = c.consignmentid +LEFT JOIN bookingserviceoptions so ON so.bookingid = cb.bookingid +GROUP BY 1; +``` + +Table and column names verified against the models: `bookingserviceoptions` +(`models/booking.go:198`), `serviceabledistricts` (`models/customer_app.go:60`), +`consignmenthistory` (`models/audit.go:109`). + +Two outcomes, both useful. If the constants are close, **there is no ETA problem +to solve** and the honest answer is to stop. If they are badly off, this query +gives the baseline error that every later rung must beat — and it may be fixable +by re-tuning the constants per service type and district, which is an afternoon's +work rather than a model. + +### 1.1 — Routing-based ETA (**the rung most likely to be the right stopping point**) + +**Better news than the first draft assumed, with a catch.** `internal/routing` +is a client for a real Valhalla-backed road-network API +(`routes.workolik.com /api/v1/optimization/doormile/sequence`), and its response +already carries durations, not just an ordering: `etaminutes`, `cumulativeeta`, +`previouskms`, `cumulativekms`, `totaleta` (`internal/routing/optimizer.go:81-95`). + +And **those values are already persisted**, on `bookingassignments` +(`models/booking.go:239-244`): `step`, `previouskms`, `cumulativekms`, +`etaminutes`, `cumulativeeta`, `sequencedat`. So a predicted-vs-actual dataset — +routed ETA at assignment time paired with the delivery event — is structurally +already being collected. No new capture plumbing is needed. + +**The catch, and it is a real one: those columns are almost certainly all zeros +in production.** Two independent gates: + +1. **`routing.BaseURL` empty disables sequencing entirely** (deliberately — + `optimizer.go:26`). It is set from `cfg.RouteOptimizerURL` at `main.go:246`, + and `ROUTE_OPTIMIZER_URL` is one of the 19 env vars missing from + `kubernetes/manifests/doormile/miletruth.yaml` — see Track A1 of the Phase 7 + plan. CLAUDE.md §8.5 says the same thing from the other side: *"Sequencing + … is not deployed yet, so riders with more than one stop come back + unsequenced (step: 0)."* +2. **`minStopsToSequence = 2`** (`optimizer.go:44`) — a rider with one stop is + never sequenced. In a courier operation a large share of assignments may be + single-stop, so even with routing switched on, routed ETAs cover only + multi-stop riders. + +**Two consequences for the plan:** + +- **Rung 1.1 has a hard prerequisite: Phase 7 Track A1.** Until + `ROUTE_OPTIMIZER_URL` is in the manifest, there is no routed duration to + calibrate, and `etaminutes` stays 0. Verify before building: + + ```sql + SELECT count(*) AS assignments, + count(*) FILTER (WHERE etaminutes > 0) AS with_routed_eta, + count(*) FILTER (WHERE step > 0) AS sequenced, + min(sequencedat), max(sequencedat) + FROM bookingassignments; + ``` + + `with_routed_eta = 0` means this rung starts by turning routing on and waiting + for data, not by fitting a calibration. + +- **Single-stop assignments need their own duration source.** Haversine × + a learned road-circuity factor per zone is the pragmatic fallback — + `haversineKM` already exists once, in `hubController.go` (do not redefine it; + CLAUDE.md §7). Alternatively call the routing API for single stops too, which + is a change to `minStopsToSequence`'s rationale and should be decided + explicitly rather than assumed. + +With a routed duration in hand, the calibration is a grouped median over history +and not machine learning: + +``` +eta = routed_duration × calibration[zone, hour_bucket, weekday] + handling_time[hub] +``` + +One SQL query, refreshed nightly. Interpretable, debuggable, no training +infrastructure, no model server — and because `etaminutes` is already stored per +assignment, the calibration is fitted on the system's own past predictions +against its own actuals, which is the cleanest possible training signal. + +**Only climb past this rung if 1.1 is measurably insufficient.** For a +single-city hub-based courier, it very often isn't. + +### 3a. Where the calibrated ETA can actually be applied — **decision needed** + +Found while implementing, and it changes the integration plan. + +**At booking-create time there is no routed duration.** The sequence is: booking +created → `estimateddeliveryat` written from the promise table → rider assigned +→ `internal/routing` sequences and writes `etaminutes`. The routed number +arrives *after* the promise has been set and shown. + +So wiring `prediction.ETAMinutes` into `adminController.go:2738` or +`cxPickupFanout.go:224` would return `false` on every call, forever — not +because the calibration is cold, but because `RoutedMinutes` is structurally 0 +at that moment. That is dead code, so those call sites were left alone. + +The routed ETA first exists at **sequencing time**, which means applying it is +revising a promise the customer has already been given. That is a product +decision, not a wiring one: + +| | | +|---|---| +| **Option A — refine `estimateddeliveryat`, never touch `sladueat`** | The customer's "arrives by" sharpens as the system learns more; the commitment they were given does not move. **Recommended.** | +| Option B — leave both, expose the calibrated ETA only on tracking | Nothing stored changes; the sharper number is display-only. Safest, least useful. | +| Option C — revise both | The SLA stops being a commitment. Not recommended. | + +Option A needs one call in `internal/routing` after the ETA columns are written, +plus a decision on whether `cxstage` should emit an event when a promise moves — +a customer watching the tracking page will see the time change, and silently +is probably the wrong way for that to happen. + +**This is decision 7 in §8.** Nothing should be wired until it is made. + +### 1.2 — Gradient-boosted regression (only if 1.1 is insufficient) + +Features, all already in the schema: + +| Feature | Source | +|---|---| +| haversine + routed distance | `pickuplatitude/longitude`, `deliverylatitude/longitude` | +| hour of day, weekday | `createdat` **via `EpochMillis`** (H1) | +| pickup / delivery pincode | `pickuppincode`, `deliverypincode` | +| origin / destination hub | `originhubid`, `destinationhubid` | +| chargeable weight | `chargeableweight` | +| service type | `bookingserviceoptions.servicetype` | +| attempt count | `consignments.attemptcount` | +| rider | `assignedmileruserid` (hash, not identity — see §8) | +| hub inbound load at assignment | derived from `consignmenthistory` | + +**Predict a quantile, not a mean.** An ETA shown to a customer should be the p80 +— "arrives by" — not the average, which is late half the time. Use quantile +regression or a boosted model with a quantile objective. This single choice +matters more to perceived accuracy than the model family. + +Label: `delivered_event.createdat − consignment.createdat`, both corrected for H1. + +### 1.3 — Attempt-aware ETA + +`attemptcount` exists and `MilerSkipDelivery` increments it, so failed attempts +are recorded. An ETA that ignores re-attempts is wrong for exactly the parcels +customers complain about. Worth a separate model only once 1.2 is in production +and its residuals show re-attempts as the dominant error mode. + +--- + +## 4. Phase 2 — Demand + +### 2.1 — The series + +```sql +CREATE OR REPLACE VIEW demand_daily AS +SELECT (createdat)::date AS day, -- H1 correction applied in the real view + pickuppincode, + count(*) AS bookings +FROM pickupbookings +WHERE status <> 'Cancelled' +GROUP BY 1, 2; +``` + +Decide the grain deliberately: pincode × day is what `rebalance_riders` wants, +but it is also the sparsest. Start at **city × day**, prove the pipeline, then +descend to zone only where series length supports it. + +### 2.2 — Baseline first + +Seasonal naïve — "same weekday last week" — is the baseline. It is one line of +SQL and it beats badly-configured SARIMA routinely. Any model that cannot beat it +on held-out data does not ship. + +### 2.3 — SARIMA or Prophet + +| | Choose when | +|---|---| +| **SARIMA** (`statsmodels`) | few series, weekly seasonality, want interpretable orders | +| **Prophet** | many series, holidays matter (Indian festival calendar is a real effect on courier volume), need it to work without per-series tuning | + +**Recommendation: Prophet**, for the holiday regressors. Diwali, Pongal and +regional festivals move courier volume substantially, Prophet takes a holiday +calendar as a first-class input, and it does not need per-series order selection +across dozens of pincodes. + +Validate with rolling-origin backtesting (expanding window), **never** a random +split — a random train/test split on time series leaks the future and reports an +accuracy you will not see in production. + +### 2.4 — What consumes the forecast + +Demand prediction with no consumer is a dashboard nobody opens. The honest +consumer is `rebalance_riders` (`seed.go:295`), which is itself unimplemented — +so Phase 2 should be scoped **with** that tool's executor or not at all. + +Minimum useful output: tomorrow's expected bookings per zone, plus a +staffing-gap signal against rostered riders. That is actionable; a forecast +number alone is not. + +--- + +## 5. Where this runs + +A prediction service is a **third** runtime next to the Go API and the Python +agents. Options, cheapest first: + +| Option | Shape | Cost | +|---|---|---| +| **A — SQL + nightly job** | Calibration tables computed by a Go sweeper; serving is a table lookup | no new runtime, no new image | +| **B — module inside `AI_engine`** | New package; adds `pandas`/`statsmodels`/`prophet` to the image | one runtime, image grows ~300MB | +| **C — separate service** | Own repo/image/deploy/probes | full operational cost | + +**Recommendation: A for Phase 1.1, B for Phase 2.** Rung 1.1 needs no model +server at all — it is a calibration table and a multiply, so it belongs in the +Go backend as a sweeper beside `StartPendingSweeper` (`main.go:244`). Prophet +genuinely needs Python, so Phase 2 lands in `AI_engine`. Option C only becomes +right if Phase 1.2 happens and model serving needs independent scaling. + +**Prerequisite from the Phase 7 plan:** `AI_engine` is not in Kubernetes and has +no HTTP health surface (Track C1 there). Phase 2 inherits that work — it cannot +deploy before it. + +--- + +## 6. LSTM — the condition for revisiting + +Not recommended now. The condition under which it becomes worth measuring: + +- Phase 2.3 is in production, backtested, and **losing to its own residual + structure** — i.e. Prophet's errors are autocorrelated in a way a sequence + model could capture; and +- ≥ 2 years of daily data across ≥ 50 series (≈ 36,000 observations), and +- a measured business cost to the remaining forecast error that exceeds the cost + of training infrastructure, GPU or CPU-hours, and the ongoing retraining a + neural model needs to not rot. + +All three, not any one. Until then an LSTM here would be a more expensive way to +get a worse number, and the honest recommendation is to say so rather than build +it. + +--- + +## 7. File manifest + +### Phase 0 — data readiness (no application code) + +| File | New? | +|---|---| +| `doormile_backend/docs/prediction-data-readiness.md` | **new** — the findings note | +| `doormile_backend/scratch/readiness_queries.sql` | **new** — the queries above, kept for re-running | + +### Phase 1.0 / 1.1 — measurement and routing ETA + +| File | New? | Change | +|---|---|---| +| `doormile_backend/migrations/migrate.go` | | add the `consignment_booking` view (H4), the `trip_features` view (H1 correction in one place), and the `eta_calibration` table | +| `doormile_backend/internal/prediction/calibration.go` | **new** | nightly grouped-median refresh | +| `doormile_backend/internal/prediction/eta.go` | **new** | `EstimateETA(booking) (time.Time, confidence)` | +| `doormile_backend/internal/prediction/eta_test.go` | **new** | falls back to the promise table when calibration is missing | +| `doormile_backend/internal/prediction/sweeper.go` | **new** | ticker + Redis lock, pattern from `internal/assignment/sweeper.go:88` | +| `doormile_backend/main.go:244` | | `go prediction.StartCalibrationSweeper()` | +| `doormile_backend/controllers/adminController.go:2740` | | call `prediction.EstimateETA`, keep constants as fallback | +| `doormile_backend/controllers/cxPickupFanout.go:224` | | same, keep the promise table as fallback | +| `doormile_backend/utils/epoch.go` | | read-only — the H1 conversion to reuse | + +### Phase 1.2 — learned ETA (only if 1.1 insufficient) + +| File | New? | +|---|---| +| `AI_engine/prediction/__init__.py` · `eta_model.py` · `features.py` | **new** | +| `AI_engine/prediction/train_eta.py` | **new** — offline training, writes a versioned artifact | +| `AI_engine/tests/test_eta_features.py` | **new** — H1 correction asserted on both timestamp taggings | +| `AI_engine/requirements.txt` | modify — `pandas`, `scikit-learn` or `lightgbm` | +| `doormile_backend/internal/prediction/eta.go` | modify — call the service, fall back to 1.1 | + +### Phase 2 — demand + +| File | New? | +|---|---| +| `AI_engine/prediction/demand_model.py` | **new** | +| `AI_engine/prediction/holidays_in.py` | **new** — the festival calendar | +| `AI_engine/prediction/backtest.py` | **new** — rolling-origin, never a random split | +| `AI_engine/tests/test_demand_backtest.py` | **new** — must beat seasonal-naïve to pass | +| `AI_engine/requirements.txt` | modify — `prophet` or `statsmodels` | +| `AI_engine/Dockerfile` | modify — Prophet needs a compiler toolchain | +| `doormile_backend/migrations/migrate.go` | modify — `demand_daily` view, `demand_forecast` table | +| `doormile_backend/routes/routes.go` | modify — `GET /admin/forecast/demand` (staff-only) | +| `doormile_backend/controllers/forecastController.go` | **new** | +| `doormile_backend/internal/ai/registry/seed.go:295` | modify — `rebalance_riders` once it has a consumer | +| `kubernetes/manifests/doormile/ai-engine.yaml` | modify — resources for Prophet | + +### Console (Phase 2 only, optional) + +| File | Change | +|---|---| +| `krow_talent_app/src/api/doormile/endpoints.js` | add `getDemandForecast` | +| a new forecast panel | render it — **after** a consumer exists, not before | + +--- + +## 8. Decisions needed + +0. **Is `ROUTE_OPTIMIZER_URL` set in the cluster?** If not, `etaminutes` is 0 + everywhere and rung 1.1 begins with Phase 7 Track A1 plus a data-accumulation + wait, not with a calibration. This gates more than anything else here. +1. **Is there enough history?** Phase 0 answers it. Everything else is blocked + on that number. +1b. **Do single-stop assignments get routed too?** `minStopsToSequence = 2` + excludes them today. Either lower it, or accept a haversine-based fallback for + single-stop ETAs. An explicit call, not an assumption. +2. **Does ETA need to be learned at all, or do the constants just need + re-tuning?** Rung 1.0 answers it, cheaply. +3. **p80 or mean ETA?** Recommend p80 — "arrives by" is the promise customers + hear, and a mean is late half the time. +4. **Demand grain** — city × day to start, or straight to pincode × day? + Recommend city first. +5. **Does `rebalance_riders` get an executor in the same phase?** If no, Phase 2 + produces a number nobody acts on. +6. **Rider as a feature (1.2)** — per-rider ETA adjustment is a performance + signal about a named person. Hash it, use it only in aggregate, and decide + deliberately whether it may ever surface in the console. This is a people + decision, not a modelling one. +7. **May a promise be revised after the customer has seen it?** §3a. Blocks the + last wiring step of rung 1.1 — everything else is built. Recommend Option A: + refine `estimateddeliveryat`, never move `sladueat`. + +--- + +## 9. Sequencing + +``` +Phase 7 A1 (env vars) ──> routing actually runs ──> etaminutes accumulates + │ │ + ▼ ▼ +Phase 0 (readiness) ──> [go / no-go] 1.1 calibration possible + │ + no-go ─────────────┴──> stop; re-tune constants (1.0) and revisit after go-live + │ + go ──> 1.0 measure ──> 1.1 routing ETA ──> [measure] ──> 1.2 only if needed + └──> 2.1/2.2 baseline ──> 2.3 Prophet (needs Phase 7 C1 first) +``` + +**Both tracks now depend on the Phase 7 plan**, for different reasons: rung 1.1 +needs `ROUTE_OPTIMIZER_URL` (Track A1) before routed ETAs exist at all, and +Phase 2.3 needs `AI_engine` deployable with a health surface (Track C1). Track A1 +is one manifest edit and unblocks both — do it first regardless of which track +you want. + +**Start with Phase 0 and rung 1.0.** Both are SQL, neither needs a model, and +together they either justify the rest of this plan or retire it. Rung 1.1 is the +highest-leverage item in the document and is not machine learning at all. + +The likeliest honest outcome: **1.0 + 1.1 ship, 1.2 is never needed, Phase 2 +waits for volume, and LSTM never happens.** That is a success, not a shortfall. + +--- + +## Standing constraints + +- Nothing committed, pushed or deployed without being asked. +- No migrations run against a real database without being asked — the views and + tables here are additive, but "additive" is not "has run". +- Phase 0's queries are read-only; they are safe to run, and should be run before + anything else in this document. diff --git a/internal/ai/outcomes/backfill.go b/internal/ai/outcomes/backfill.go new file mode 100644 index 0000000..ba35c8e --- /dev/null +++ b/internal/ai/outcomes/backfill.go @@ -0,0 +1,240 @@ +package outcomes + +import ( + "encoding/json" + "time" + + "doormile/constants" + "doormile/models" + "doormile/utils" + + "gorm.io/gorm" +) + +// Seeding the decision memory from history, so recall is not useless for weeks. +// +// ─── The cold-start problem this solves ──────────────────────────────────── +// +// `/internal/agent-decisions/similar` filters on `outcome IS NOT NULL`. A row +// becomes eligible only after the outcome sweeper has judged it, and a decision +// is only judged once its window has closed. So the sequence for a freshly +// switched-on memory is: +// +// switch embeddings on -> wait for stalls to happen -> wait 48h per decision +// -> the sweeper labels them -> only now does recall return anything +// +// With autonomy gates off and two decision types, that is weeks of recall +// returning [] — which reads as "retrieval does not help here" rather than +// "retrieval has nothing to retrieve yet". The first conclusion is wrong and +// expensive to un-learn. +// +// This backfills decisions from bookings whose outcome is ALREADY known. +// +// ─── What it does and does not claim ─────────────────────────────────────── +// +// A backfilled row is not a decision the engine made. It is a record of a +// situation that occurred and how it ended, shaped so the retrieval path can +// use it as precedent. That distinction is recorded honestly: +// +// decision.action = "none" — nothing was decided; nobody intervened +// decision.source = "backfill" — so these are distinguishable forever +// reasoning = states plainly that this is historical, not a decision +// +// Why that matters: as_prompt_block renders `action` to the model. Writing a +// plausible-looking action here would teach the model that an action it never +// took produced the outcome that followed — which is worse than no memory. +// "none" is honest: this is what happened when nothing was done. +// +// ─── It writes no embeddings ─────────────────────────────────────────────── +// +// Embedding is the engine's job (AI_engine/core/embeddings.py) and needs the +// provider key, which this process does not have. Backfilled rows land with +// NULL context_embedding and are invisible to similarity search until +// something embeds them. That is deliberate: a backfill that silently created +// un-embedded rows AND claimed to have seeded the memory would be the worse +// failure. BackfillStats reports the count so the caller knows what is owed. + +// BackfillStats is what one run produced. +type BackfillStats struct { + Scanned int `json:"scanned"` + Inserted int `json:"inserted"` + Skipped int `json:"skipped"` + // NeedsEmbedding is Inserted — every backfilled row still needs a vector + // before it can be retrieved. Surfaced separately so it cannot be missed. + NeedsEmbedding int `json:"needsembedding"` +} + +// historicalRow is one past booking whose ending is known. +// +// Joins through consignment_booking, never consignments.bookingid — that column +// does not exist, and the legacy pickupbookings.consignmentid link names only +// the first order of a multi-destination pickup (hazard H4). +type historicalRow struct { + Bookingid int + Tenantid *uint64 + Deliverypincode string + Status string + Createdat time.Time + Deliveredat *time.Time + Sladueat *time.Time + Attemptcount int + Chargeableweight float64 +} + +const historicalSQL = ` +SELECT pb.bookingid, + pb.tenantid AS tenantid, + COALESCE(c.deliverypincode, '') AS deliverypincode, + COALESCE(c.status, pb.status) AS status, + c.createdat AS createdat, + del.createdat AS deliveredat, + c.sladueat AS sladueat, + COALESCE(c.attemptcount, 0) AS attemptcount, + COALESCE(c.chargeableweight, 0) AS chargeableweight +FROM consignments c +JOIN consignment_booking cb ON cb.consignmentid = c.consignmentid +JOIN pickupbookings pb ON pb.bookingid = cb.bookingid +LEFT JOIN ( + SELECT DISTINCT ON (consignmentid) consignmentid, createdat + FROM consignmenthistory + WHERE eventstatus = ? + ORDER BY consignmentid, createdat ASC +) del ON del.consignmentid = c.consignmentid +WHERE c.deletedat IS NULL + -- Only parcels whose story has ended. An in-flight parcel has no outcome to + -- learn from, and guessing one is exactly what this must not do. + AND (del.createdat IS NOT NULL OR c.status IN ?) + -- Not already backfilled or decided for. The uniqueness is on + -- (decision_type, booking_id), enforced here rather than by a constraint + -- because real decisions legitimately repeat for one booking. + AND NOT EXISTS ( + SELECT 1 FROM agent_decisions d + WHERE d.booking_id = pb.bookingid AND d.decision_type = ? + ) +ORDER BY c.createdat DESC +LIMIT ? +` + +// BackfillDecisionType is its own type, kept separate from the engine's +// `stall_response` and `assignment_failure`. Recall is per-type, so backfilled +// precedent is retrievable on purpose and never silently mixed into a type the +// engine thinks it authored. +const BackfillDecisionType = "historical_delivery" + +// Backfill writes historical precedent rows. Idempotent: a booking that already +// has a decision of this type is skipped, so re-running adds only what is new. +// +// limit bounds one run — this scans delivery history, which is the largest +// table pair in the database. +func Backfill(gdb *gorm.DB, limit int) (BackfillStats, error) { + if gdb == nil { + return BackfillStats{}, nil + } + if limit <= 0 { + limit = 2000 + } + + terminal := []string{ + constants.ConsignmentDelivered, + "Returned_to_Sender", + "Missing", + "Damaged", + } + + var rows []historicalRow + if err := gdb.Raw(historicalSQL, + constants.ConsignmentDelivered, terminal, BackfillDecisionType, limit, + ).Scan(&rows).Error; err != nil { + return BackfillStats{}, err + } + + stats := BackfillStats{Scanned: len(rows)} + batch := make([]models.AgentDecision, 0, len(rows)) + + for _, r := range rows { + outcome := OutcomeFailure + switch { + case r.Deliveredat == nil: + // Terminal but never delivered: returned, lost or damaged. + outcome = OutcomeFailure + case r.Sladueat != nil && r.Deliveredat.After(*r.Sladueat): + // Delivered late. Counting this a success would teach that any + // eventual delivery is a good outcome — the same trap the + // stall_response rule avoids. + outcome = OutcomeFailure + default: + outcome = OutcomeSuccess + } + + // The facts shape mirrors what AI_engine puts in context.facts, so an + // embedding of a backfilled row sits in the same space as a real one. + // If these diverge, retrieval returns neighbours that are near in + // vector space for the wrong reasons. + facts := map[string]any{ + "booking_id": r.Bookingid, + "delivery_pincode": r.Deliverypincode, + "attempt_count": r.Attemptcount, + "chargeable_weight": r.Chargeableweight, + "final_status": r.Status, + } + if r.Deliveredat != nil { + facts["hours_to_deliver"] = int(r.Deliveredat.Sub(r.Createdat).Hours()) + } + + contextJSON, err := json.Marshal(map[string]any{ + "facts": facts, + "model": "none", + "source": "backfill", + }) + if err != nil { + stats.Skipped++ + continue + } + decisionJSON, err := json.Marshal(map[string]any{ + // Honest: nobody decided anything. See the package comment — a + // plausible-looking action here would be a fabricated lesson. + "action": "none", + "confidence": 0.0, + "source": "backfill", + }) + if err != nil { + stats.Skipped++ + continue + } + + recordedAt := utils.DBNow() + batch = append(batch, models.AgentDecision{ + DecisionType: BackfillDecisionType, + BookingID: u64(r.Bookingid), + TenantID: r.Tenantid, + Context: string(contextJSON), + Decision: string(decisionJSON), + Reasoning: "Historical outcome backfilled from delivery records. No agent decision was made for this booking; this row records what happened when nothing intervened.", + Outcome: &outcome, + OutcomeRecordedAt: &recordedAt, + CreatedAt: r.Createdat, + }) + } + + if len(batch) == 0 { + return stats, nil + } + if err := gdb.CreateInBatches(&batch, 200).Error; err != nil { + return stats, err + } + stats.Inserted = len(batch) + stats.NeedsEmbedding = len(batch) + + utils.Info("outcomes: backfilled historical precedent", + "scanned", stats.Scanned, "inserted", stats.Inserted, "skipped", stats.Skipped, + "note", "rows have no embedding yet and are not retrievable until one is written") + return stats, nil +} + +func u64(n int) *uint64 { + if n <= 0 { + return nil + } + v := uint64(n) + return &v +} diff --git a/internal/ai/outcomes/rules.go b/internal/ai/outcomes/rules.go new file mode 100644 index 0000000..ec39d6a --- /dev/null +++ b/internal/ai/outcomes/rules.go @@ -0,0 +1,158 @@ +// Package outcomes decides, after the fact, whether an agent's decision worked. +// +// This is the half of the decision memory that was missing, and without it the +// other half does nothing. `/internal/agent-decisions/similar` filters on +// `outcome IS NOT NULL`, so until something judges a decision it is invisible +// to retrieval. Embeddings could flow for months and every recall would still +// come back empty. +// +// A decision is not precedent because it was made. It is precedent because we +// know how it turned out. +// +// ─── What "worked" means ────────────────────────────────────────────────── +// +// Two decision types exist today, from exactly two call sites in the engine: +// +// stall_response AI_engine/agents/exception_agent.py:456 +// assignment_failure AI_engine/agents/dispatch_agent.py:335 +// +// Each gets its own definition below. A third type appearing without a rule +// here is left pending rather than guessed at — a wrong label is worse than no +// label, because it teaches the model confidently. +package outcomes + +import ( + "os" + "strconv" + "strings" + "time" + + "doormile/utils" +) + +// The outcome vocabulary. Stored in agent_decisions.outcome (varchar 30) and +// read back by the Insights tab, which groups by it. +const ( + // OutcomeSuccess — the thing the decision was trying to achieve happened. + OutcomeSuccess = "success" + // OutcomeFailure — it did not. + OutcomeFailure = "failure" + // OutcomeUnknown — the window closed without enough evidence either way. + // Deliberately recorded rather than left pending: a pending row is + // retried every sweep forever, and an unjudgeable decision should stop + // costing a scan. It is excluded from retrieval the same as pending, + // because `outcome IS NOT NULL` is not the only filter that matters — + // see judgeable(). + OutcomeUnknown = "unknown" +) + +const ( + defaultOutcomeWindowHours = 48 + // A decision younger than this is left alone: the booking it concerns is + // probably still in flight, and judging it now would record a failure for + // something that simply has not finished. + minDecisionAge = 30 * time.Minute + // Caps one sweep's work; the rest are next sweep's. + sweepBatch = 500 +) + +// OutcomeWindow is how long after a decision its result is judged. +// AGENT_OUTCOME_WINDOW_HOURS, default 48. +// +// The window is a real tradeoff, not a tuning knob. Too short and a parcel +// that was always going to take three days is recorded as a failure of the +// decision rather than of the promise. Too long and the memory learns slowly. +// 48h matches the Standard service SLA (36h) with headroom. +func OutcomeWindow() time.Duration { + if v := strings.TrimSpace(os.Getenv("AGENT_OUTCOME_WINDOW_HOURS")); v != "" { + if n, err := strconv.Atoi(v); err == nil && n > 0 { + return time.Duration(n) * time.Hour + } + utils.Warn("AGENT_OUTCOME_WINDOW_HOURS is not a positive integer, using the default", + "value", v, "default_hours", defaultOutcomeWindowHours) + } + return defaultOutcomeWindowHours * time.Hour +} + +// RetentionDays bounds how long decisions are kept. Longer than the 30 days +// aiagentruns keeps, because old precedent is the whole point of this table — +// but not unbounded, which is what it was. +func RetentionDays() int { + if v := strings.TrimSpace(os.Getenv("AGENT_DECISION_RETENTION_DAYS")); v != "" { + if n, err := strconv.Atoi(v); err == nil && n > 0 { + return n + } + } + return 180 +} + +// bookingFacts is what the sweeper reads about the booking a decision concerned. +// Timestamps come from columns written with CURRENT_TIMESTAMP defaults, so they +// are consistent with each other and their differences are correct regardless +// of the IST-digits-labelled-UTC convention (utils.DBNow) — the same reasoning +// internal/prediction's calibration relies on. +type bookingFacts struct { + Bookingid int + Status string + Assigned bool + DecidedAt time.Time + DeliveredAt *time.Time + SLADueAt *time.Time + AssignedAt *time.Time + Cancelled bool +} + +// judge applies the per-type rule. Returns the outcome and whether the decision +// is judgeable at all; a false means leave it pending for now. +func judge(decisionType string, f bookingFacts, now time.Time, window time.Duration) (string, bool) { + age := now.Sub(f.DecidedAt) + if age < minDecisionAge { + return "", false // too soon; the booking is still in flight + } + + switch decisionType { + case "stall_response": + // The agent intervened because a rider had stopped making progress. + // It worked if the parcel reached the customer, and reached them + // within the promise that was in force. + if f.Cancelled { + return OutcomeFailure, true + } + if f.DeliveredAt != nil { + if f.SLADueAt != nil && f.DeliveredAt.After(*f.SLADueAt) { + // Delivered, but late. Counting this as success would teach + // the model that any eventual delivery vindicates the action. + return OutcomeFailure, true + } + return OutcomeSuccess, true + } + if age >= window { + // Window closed, never delivered, not cancelled — stuck. + return OutcomeFailure, true + } + return "", false + + case "assignment_failure": + // The agent reasoned about why no rider could be found. It worked if + // the booking subsequently got one. + if f.Cancelled { + return OutcomeFailure, true + } + if f.Assigned && f.AssignedAt != nil && f.AssignedAt.After(f.DecidedAt) { + return OutcomeSuccess, true + } + if age >= window { + return OutcomeFailure, true + } + return "", false + + default: + // An unrecognised decision type. Judge it unknown once the window has + // closed so it stops being rescanned, but never guess success or + // failure — a wrong label is worse than no label. + if age >= window { + return OutcomeUnknown, true + } + return "", false + } +} diff --git a/internal/ai/outcomes/rules_test.go b/internal/ai/outcomes/rules_test.go new file mode 100644 index 0000000..58ad619 --- /dev/null +++ b/internal/ai/outcomes/rules_test.go @@ -0,0 +1,214 @@ +package outcomes + +import ( + "testing" + "time" +) + +func at(h int) time.Time { + return time.Date(2026, 10, 5, h, 0, 0, 0, time.UTC) +} + +func tp(t time.Time) *time.Time { return &t } + +const window = 48 * time.Hour + +// ─── Too soon to judge ───────────────────────────────────────────────────── + +func TestYoungDecisionIsLeftPending(t *testing.T) { + decided := at(10) + for _, dt := range []string{"stall_response", "assignment_failure", "something_new"} { + _, ok := judge(dt, bookingFacts{DecidedAt: decided}, decided.Add(5*time.Minute), window) + if ok { + t.Errorf("%s: judged a 5-minute-old decision; the booking is still in flight", dt) + } + } +} + +func TestPendingWhileInsideTheWindow(t *testing.T) { + decided := at(10) + // An hour in: no delivery yet, but the window has not closed. Recording + // failure here would blame the decision for a parcel still on its way. + if _, ok := judge("stall_response", bookingFacts{DecidedAt: decided}, decided.Add(time.Hour), window); ok { + t.Error("stall_response judged before its window closed") + } + if _, ok := judge("assignment_failure", bookingFacts{DecidedAt: decided}, decided.Add(time.Hour), window); ok { + t.Error("assignment_failure judged before its window closed") + } +} + +// ─── stall_response ──────────────────────────────────────────────────────── + +func TestStallDeliveredOnTimeIsSuccess(t *testing.T) { + decided := at(10) + f := bookingFacts{ + DecidedAt: decided, + DeliveredAt: tp(decided.Add(3 * time.Hour)), + SLADueAt: tp(decided.Add(12 * time.Hour)), + } + got, ok := judge("stall_response", f, decided.Add(4*time.Hour), window) + if !ok || got != OutcomeSuccess { + t.Errorf("got %q ok=%v, want success", got, ok) + } +} + +// The case that matters most: counting any eventual delivery as success would +// teach the model that the action is always vindicated, which is exactly the +// wrong lesson. +func TestStallDeliveredLateIsFailure(t *testing.T) { + decided := at(10) + f := bookingFacts{ + DecidedAt: decided, + DeliveredAt: tp(decided.Add(20 * time.Hour)), + SLADueAt: tp(decided.Add(12 * time.Hour)), + } + got, ok := judge("stall_response", f, decided.Add(21*time.Hour), window) + if !ok || got != OutcomeFailure { + t.Errorf("got %q ok=%v, want failure — delivered 8h past SLA", got, ok) + } +} + +func TestStallDeliveredWithNoSLAIsSuccess(t *testing.T) { + decided := at(10) + // No sladueat recorded (an older row). Delivery is the best evidence + // available and there is nothing to call it late against. + f := bookingFacts{DecidedAt: decided, DeliveredAt: tp(decided.Add(5 * time.Hour))} + got, ok := judge("stall_response", f, decided.Add(6*time.Hour), window) + if !ok || got != OutcomeSuccess { + t.Errorf("got %q ok=%v, want success", got, ok) + } +} + +func TestStallNeverDeliveredAfterWindowIsFailure(t *testing.T) { + decided := at(10) + got, ok := judge("stall_response", bookingFacts{DecidedAt: decided}, decided.Add(window+time.Hour), window) + if !ok || got != OutcomeFailure { + t.Errorf("got %q ok=%v, want failure", got, ok) + } +} + +func TestStallCancelledIsFailureImmediately(t *testing.T) { + decided := at(10) + f := bookingFacts{DecidedAt: decided, Cancelled: true} + // Cancellation is conclusive — no need to wait out the window. + got, ok := judge("stall_response", f, decided.Add(time.Hour), window) + if !ok || got != OutcomeFailure { + t.Errorf("got %q ok=%v, want failure on cancellation", got, ok) + } +} + +// ─── assignment_failure ──────────────────────────────────────────────────── + +func TestAssignmentLaterAssignedIsSuccess(t *testing.T) { + decided := at(10) + f := bookingFacts{ + DecidedAt: decided, + Assigned: true, + AssignedAt: tp(decided.Add(2 * time.Hour)), + } + got, ok := judge("assignment_failure", f, decided.Add(3*time.Hour), window) + if !ok || got != OutcomeSuccess { + t.Errorf("got %q ok=%v, want success", got, ok) + } +} + +// A rider assigned BEFORE the decision is not evidence the decision worked — +// it is the assignment that was already there. Without the ordering check, +// every reassignment decision would read as an instant success. +func TestAssignmentPredatingTheDecisionIsNotSuccess(t *testing.T) { + decided := at(10) + f := bookingFacts{ + DecidedAt: decided, + Assigned: true, + AssignedAt: tp(decided.Add(-2 * time.Hour)), + } + got, ok := judge("assignment_failure", f, decided.Add(time.Hour), window) + if ok && got == OutcomeSuccess { + t.Error("counted a pre-existing assignment as the decision's success") + } +} + +func TestAssignmentNeverAssignedAfterWindowIsFailure(t *testing.T) { + decided := at(10) + got, ok := judge("assignment_failure", bookingFacts{DecidedAt: decided}, decided.Add(window+time.Hour), window) + if !ok || got != OutcomeFailure { + t.Errorf("got %q ok=%v, want failure", got, ok) + } +} + +func TestAssignmentCancelledIsFailure(t *testing.T) { + decided := at(10) + f := bookingFacts{DecidedAt: decided, Cancelled: true} + got, ok := judge("assignment_failure", f, decided.Add(time.Hour), window) + if !ok || got != OutcomeFailure { + t.Errorf("got %q ok=%v, want failure", got, ok) + } +} + +// ─── Unknown types are never guessed ─────────────────────────────────────── + +func TestUnknownTypeIsNeverSuccessOrFailure(t *testing.T) { + decided := at(10) + // Inside the window: pending. + if _, ok := judge("a_new_decision_type", bookingFacts{DecidedAt: decided}, decided.Add(time.Hour), window); ok { + t.Error("judged an unknown decision type inside its window") + } + // Past it: unknown, so it stops being rescanned — but never a label that + // would teach the model something nobody defined. + got, ok := judge("a_new_decision_type", bookingFacts{DecidedAt: decided}, decided.Add(window+time.Hour), window) + if !ok || got != OutcomeUnknown { + t.Errorf("got %q ok=%v, want unknown", got, ok) + } +} + +// A delivered unknown type must still not be called a success: the rule for +// what success means for that type does not exist yet. +func TestUnknownTypeWithDeliveryIsStillUnknown(t *testing.T) { + decided := at(10) + f := bookingFacts{DecidedAt: decided, DeliveredAt: tp(decided.Add(time.Hour))} + got, ok := judge("a_new_decision_type", f, decided.Add(window+time.Hour), window) + if !ok || got != OutcomeUnknown { + t.Errorf("got %q ok=%v, want unknown", got, ok) + } +} + +// ─── Configuration ───────────────────────────────────────────────────────── + +func TestOutcomeWindowDefaultAndOverride(t *testing.T) { + t.Setenv("AGENT_OUTCOME_WINDOW_HOURS", "") + if got := OutcomeWindow(); got != defaultOutcomeWindowHours*time.Hour { + t.Errorf("default window = %v, want %v", got, defaultOutcomeWindowHours*time.Hour) + } + t.Setenv("AGENT_OUTCOME_WINDOW_HOURS", "12") + if got := OutcomeWindow(); got != 12*time.Hour { + t.Errorf("window = %v, want 12h", got) + } + // Garbage falls back rather than producing a zero window, which would + // judge every decision the instant it passed minDecisionAge. + t.Setenv("AGENT_OUTCOME_WINDOW_HOURS", "not-a-number") + if got := OutcomeWindow(); got != defaultOutcomeWindowHours*time.Hour { + t.Errorf("window on garbage = %v, want the default", got) + } + t.Setenv("AGENT_OUTCOME_WINDOW_HOURS", "0") + if got := OutcomeWindow(); got != defaultOutcomeWindowHours*time.Hour { + t.Errorf("window on 0 = %v, want the default", got) + } +} + +func TestRetentionDaysDefaultAndOverride(t *testing.T) { + t.Setenv("AGENT_DECISION_RETENTION_DAYS", "") + if got := RetentionDays(); got != 180 { + t.Errorf("default retention = %d, want 180", got) + } + t.Setenv("AGENT_DECISION_RETENTION_DAYS", "90") + if got := RetentionDays(); got != 90 { + t.Errorf("retention = %d, want 90", got) + } + // Retention must be longer than aiagentruns' 30 days, because old + // precedent is the point of this table. Not enforced in code — asserted + // here so a future change to the default has to confront it. + t.Setenv("AGENT_DECISION_RETENTION_DAYS", "") + if RetentionDays() <= 30 { + t.Error("decision retention is no longer than telemetry retention; precedent will be purged before it is useful") + } +} diff --git a/internal/ai/outcomes/sweeper.go b/internal/ai/outcomes/sweeper.go new file mode 100644 index 0000000..79b0a0a --- /dev/null +++ b/internal/ai/outcomes/sweeper.go @@ -0,0 +1,226 @@ +package outcomes + +import ( + "context" + "os" + "strconv" + "strings" + "time" + + "doormile/constants" + "doormile/db" + "doormile/utils" + + "gorm.io/gorm" +) + +// The outcome sweeper. +// +// Same shape as internal/assignment/sweeper.go and internal/prediction/sweeper.go: +// a ticker, a recover() per tick, and a Redis lock that expires before the next +// tick so one replica of three does the work. Without Redis every replica +// sweeps, which is wasteful but correct — each update is idempotent and scoped +// to rows that are still pending. + +const ( + defaultOutcomeSweepSeconds = 900 // 15m + minOutcomeSweepSeconds = 120 + outcomeLockKey = "ai:outcome-sweep:lock" +) + +func sweepInterval() time.Duration { + if v := strings.TrimSpace(os.Getenv("AGENT_OUTCOME_SWEEP_SECONDS")); v != "" { + if n, err := strconv.Atoi(v); err == nil && n >= 0 { + if n > 0 && n < minOutcomeSweepSeconds { + n = minOutcomeSweepSeconds + } + return time.Duration(n) * time.Second + } + utils.Warn("AGENT_OUTCOME_SWEEP_SECONDS is not a non-negative integer, using the default", + "value", v, "default_seconds", defaultOutcomeSweepSeconds) + } + return defaultOutcomeSweepSeconds * time.Second +} + +// PruneFindings is set at boot by main.go to controllers.PruneAIFindings. +// A function variable rather than a direct call because controllers imports +// most of the codebase, and this package is imported BY controllers' siblings — +// calling it directly would be an import cycle. +var PruneFindings func(retentionDays int) (int, error) + +// StartOutcomeSweeper judges pending decisions on a timer, and prunes old ones. +// Call once at boot, in a goroutine. +func StartOutcomeSweeper() { + interval := sweepInterval() + if interval == 0 { + utils.Info("OutcomeSweeper: disabled (AGENT_OUTCOME_SWEEP_SECONDS=0)") + return + } + utils.Info("OutcomeSweeper: started", + "interval", interval.String(), "window", OutcomeWindow().String(), + "retention_days", RetentionDays()) + + ticker := time.NewTicker(interval) + defer ticker.Stop() + for range ticker.C { + sweepOnce(interval) + } +} + +func sweepOnce(interval time.Duration) { + defer func() { + if r := recover(); r != nil { + utils.Error("OutcomeSweeper: panic recovered", "error", r) + } + }() + if db.DB == nil { + return + } + if db.Rdb != nil { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + ttl := interval - 30*time.Second + if ttl <= 0 { + ttl = interval / 2 + } + got, err := db.Rdb.SetNX(ctx, outcomeLockKey, "1", ttl).Result() + cancel() + if err == nil && !got { + return + } + } + + judged, err := JudgePending(db.DB, time.Now()) + if err != nil { + utils.Error("OutcomeSweeper: judging failed", "error", err) + } + pruned, err := Prune(db.DB) + if err != nil { + utils.Error("OutcomeSweeper: prune failed", "error", err) + } + // Skill findings share this tick rather than carrying their own timer: + // one more table to keep tidy, not one more goroutine. Injected so this + // package does not import controllers (which imports everything). + findingsPruned := 0 + if PruneFindings != nil { + if n, err := PruneFindings(RetentionDays()); err != nil { + utils.Error("OutcomeSweeper: finding prune failed", "error", err) + } else { + findingsPruned = n + } + } + if judged > 0 || pruned > 0 || findingsPruned > 0 { + utils.Info("OutcomeSweeper: swept", + "judged", judged, "decisions_pruned", pruned, "findings_pruned", findingsPruned) + } +} + +// pendingRow is one unjudged decision joined to the booking it concerned. +// +// The join runs through pickupbookings on the decision's own booking_id, which +// the engine supplies — not through consignments, which has no bookingid column +// (the hazard CLAUDE.md §8.5 and docs/prediction-plan.md H4 describe). The +// delivered timestamp therefore comes from consignmenthistory via the +// consignment_booking view, the one place that resolution is written down. +type pendingRow struct { + ID uint64 + DecisionType string + Bookingid *int + Status string + Assignedat *time.Time + Assigned bool + Createdat time.Time + Deliveredat *time.Time + Sladueat *time.Time +} + +const pendingSQL = ` +SELECT d.id AS id, + d.decision_type AS decision_type, + d.booking_id AS bookingid, + COALESCE(pb.status, '') AS status, + ba.assignedat AS assignedat, + (pb.assignedmileruserid IS NOT NULL) AS assigned, + d.created_at AS createdat, + del.createdat AS deliveredat, + con.sladueat AS sladueat +FROM agent_decisions d +LEFT JOIN pickupbookings pb ON pb.bookingid = d.booking_id +LEFT JOIN ( + SELECT DISTINCT ON (bookingid) bookingid, assignedat + FROM bookingassignments + ORDER BY bookingid, assignedat DESC NULLS LAST, bookingassignmentid DESC +) ba ON ba.bookingid = d.booking_id +LEFT JOIN consignment_booking cb ON cb.bookingid = d.booking_id +LEFT JOIN consignments con ON con.consignmentid = cb.consignmentid +LEFT JOIN ( + SELECT DISTINCT ON (consignmentid) consignmentid, createdat + FROM consignmenthistory + WHERE eventstatus = ? + ORDER BY consignmentid, createdat ASC +) del ON del.consignmentid = cb.consignmentid +WHERE d.outcome IS NULL +ORDER BY d.created_at ASC +LIMIT ? +` + +// JudgePending labels every pending decision it can and returns how many it +// wrote. A decision it cannot judge yet is left pending for the next sweep. +func JudgePending(gdb *gorm.DB, now time.Time) (int, error) { + if gdb == nil { + return 0, nil + } + + var rows []pendingRow + if err := gdb.Raw(pendingSQL, constants.ConsignmentDelivered, sweepBatch).Scan(&rows).Error; err != nil { + return 0, err + } + + window := OutcomeWindow() + judged := 0 + for _, r := range rows { + facts := bookingFacts{ + Status: r.Status, + Assigned: r.Assigned, + DecidedAt: r.Createdat, + DeliveredAt: r.Deliveredat, + SLADueAt: r.Sladueat, + AssignedAt: r.Assignedat, + Cancelled: r.Status == constants.BookingCancelled, + } + if r.Bookingid != nil { + facts.Bookingid = *r.Bookingid + } + + outcome, ok := judge(r.DecisionType, facts, now, window) + if !ok { + continue + } + // Guarded on outcome IS NULL so two replicas sweeping concurrently + // cannot overwrite each other, and a decision is judged exactly once. + res := gdb.Exec( + `UPDATE agent_decisions SET outcome = ?, outcome_recorded_at = ? WHERE id = ? AND outcome IS NULL`, + outcome, utils.DBNow(), r.ID, + ) + if res.Error != nil { + utils.Warn("OutcomeSweeper: could not record outcome", "id", r.ID, "error", res.Error) + continue + } + judged += int(res.RowsAffected) + } + return judged, nil +} + +// Prune drops decisions past the retention window. agent_decisions had no +// retention at all while aiagentruns purged at 30 days — and this is the table +// the similarity query scans, so unbounded growth degrades every recall. +func Prune(gdb *gorm.DB) (int, error) { + if gdb == nil { + return 0, nil + } + cutoff := utils.DBNow().AddDate(0, 0, -RetentionDays()) + res := gdb.Exec(`DELETE FROM agent_decisions WHERE created_at < ?`, cutoff) + if res.Error != nil { + return 0, res.Error + } + return int(res.RowsAffected), nil +} diff --git a/internal/ai/registry/registry_test.go b/internal/ai/registry/registry_test.go index d480324..653ca93 100644 --- a/internal/ai/registry/registry_test.go +++ b/internal/ai/registry/registry_test.go @@ -466,8 +466,17 @@ func TestSkillsWithNoDataSourceShipDisabled(t *testing.T) { } } -// Only notify_riders has an executor in the console. Every other console write -// must say REVIEW ONLY, or Agent Studio would advertise an action that cannot run. +// Four console verbs have executors now: notify_riders, alert_low_battery_rider, +// assign_riders and trigger_auto_dispatch (the last two share the batch-assign +// solver behind POST /admin/bookings/batch-assign). Every console write that +// still has none must say REVIEW ONLY, or Agent Studio would advertise an +// action that cannot run. +// +// The invariant reads Implementedat, not a list kept here: a tool whose +// Implementedat still points into lib/assistant/skills/ has no executor, and +// one pointing at lib/assistant/agent/actions.js does. That is why wiring an +// executor means moving Implementedat — the test cannot be satisfied by +// editing the description alone. func TestConsoleWritesWithoutExecutorSayReviewOnly(t *testing.T) { for _, tl := range SeedTools { if !strings.HasPrefix(tl.Implementedat, consoleSrc+"lib/assistant/skills/") && !strings.Contains(tl.Implementedat, "(no executor)") { diff --git a/internal/ai/registry/seed.go b/internal/ai/registry/seed.go index 45231ef..3716d95 100644 --- a/internal/ai/registry/seed.go +++ b/internal/ai/registry/seed.go @@ -168,21 +168,21 @@ var SeedTools = []models.AITool{ Description: "Message the riders on a finding's orders. Runs only when an operator clicks it; partial success is reported as partial.", Target: "doormile_backend POST /admin/milers/:id/notify", Implementedat: consoleSrc + "lib/assistant/agent/actions.js", Inputschema: open}, {Toolname: "assign_riders", Kind: KindWrite, Requiresconfirmation: true, - Description: "REVIEW ONLY. Would assign a finding's orders to riders, but POST /hub/bookings/batch-assign accepts hub-staff logins only, so the console cannot run it.", Target: "doormile_backend POST /hub/bookings/batch-assign (hub staff only)", - Implementedat: consoleSrc + "lib/assistant/agent/actions.js (no executor)", Inputschema: open}, + Description: "Assign a finding's orders to riders. Greedy nearest on-duty rider, capped per rider, committed server-side — there is no preview step, so the operator's click is the gate.", Target: "doormile_backend POST /admin/bookings/batch-assign", + Implementedat: consoleSrc + "lib/assistant/agent/actions.js", Inputschema: open}, {Toolname: "enforce_otp_verification", Kind: KindWrite, Requiresconfirmation: true, Description: "REVIEW ONLY. Flag a high-value COD order as requiring the receiver's OTP at handover. No executor.", Target: "none yet", Implementedat: consoleSrc + "lib/assistant/skills/definitions/HighValueCodSkill.js", Inputschema: obj(map[string]map[string]string{"bookingId": integer("Booking to flag")}, "bookingId")}, {Toolname: "alert_low_battery_rider", Kind: KindNotify, Requiresconfirmation: true, - Description: "REVIEW ONLY. Tell a rider on a low battery to charge or report to the nearest hub. No executor.", Target: "doormile_backend POST /admin/milers/:id/notify", - Implementedat: consoleSrc + "lib/assistant/skills/definitions/RiderBatterySafetySkill.js", Inputschema: obj(map[string]map[string]string{"milerId": integer("Rider to alert")}, "milerId")}, + Description: "Tell a rider on a low battery to charge or report to the nearest hub.", Target: "doormile_backend POST /admin/milers/:id/notify", + Implementedat: consoleSrc + "lib/assistant/agent/actions.js", Inputschema: obj(map[string]map[string]string{"milerId": integer("Rider to alert")}, "milerId")}, {Toolname: "dispatch_hub_idle_parcels", Kind: KindWrite, Requiresconfirmation: true, Description: "REVIEW ONLY. Send an idle rider to collect parcels dwelling at a hub. No executor.", Target: "none yet", Implementedat: consoleSrc + "lib/assistant/skills/definitions/HubCongestionSkill.js", Inputschema: obj(map[string]map[string]string{"hubId": str("Hub where parcels are waiting"), "milerId": integer("Idle rider")}, "hubId", "milerId")}, {Toolname: "trigger_auto_dispatch", Kind: KindWrite, Requiresconfirmation: true, - Description: "REVIEW ONLY. Auto-assign orders that have waited too long for dispatch. No executor.", Target: "none yet", - Implementedat: consoleSrc + "lib/assistant/skills/definitions/LateDispatchSkill.js", Inputschema: open}, + Description: "Auto-assign orders that have waited too long for dispatch. The same batch-assign action as assign_riders, under the name LateDispatchSkill proposes it by.", Target: "doormile_backend POST /admin/bookings/batch-assign", + Implementedat: consoleSrc + "lib/assistant/agent/actions.js", Inputschema: open}, {Toolname: "enforce_cash_handoff", Kind: KindWrite, Requiresconfirmation: true, Description: "REVIEW ONLY. Route a rider carrying too much COD via the nearest hub. No executor.", Target: "none yet", Implementedat: consoleSrc + "lib/assistant/skills/definitions/CashExposureSkill.js", Inputschema: open}, diff --git a/internal/assignment/crm_assignment.go b/internal/assignment/crm_assignment.go index 792bd7c..60f615c 100644 --- a/internal/assignment/crm_assignment.go +++ b/internal/assignment/crm_assignment.go @@ -26,7 +26,25 @@ const ( maxRetries = 5 retryDelay = 2 * time.Minute geoRadiusKm = 10.0 - geoMaxCount = 10 + + // How many GEO members to fetch per search. + // + // Raised from 10, and the reason matters. GEOSEARCH returns the N NEAREST + // members, and eligibility (GPS freshness, availability, the active cap) is + // applied AFTERWARDS. So any member that is near but not eligible consumes + // one of the N slots and pushes a usable rider out of the result entirely. + // + // Stale members were the worst case: nothing removed a rider from the set + // when they went off duty, so riders who finished weeks ago sat frozen near + // the hub where most pickups originate — the nearest members there are. + // Ten of those filled every slot, all ten were discarded, and the pool + // collapsed to one rider who then took every order in the city. + // + // MilerEndDuty now removes riders (internal/milergeo.Remove), which fixes + // it going forward. This is the defence for members ALREADY in a live + // Redis, and for any future reason a nearby rider turns out ineligible. + // 50 members is a cheap read and leaves room for the filters. + geoMaxCount = 50 // defaultMaxActive is how many open stops one miler may hold at once. // Override with MILER_MAX_ACTIVE_BOOKINGS — how many parcels a rider can diff --git a/internal/milergeo/milergeo.go b/internal/milergeo/milergeo.go index 04a86e9..7cbf714 100644 --- a/internal/milergeo/milergeo.go +++ b/internal/milergeo/milergeo.go @@ -15,6 +15,7 @@ import ( "context" "fmt" "reflect" + "strconv" "strings" "sync/atomic" @@ -31,6 +32,35 @@ type Client interface { GeoAdd(ctx context.Context, key string, geoLocation ...*redis.GeoLocation) *redis.IntCmd GeoSearchLocation(ctx context.Context, key string, q *redis.GeoSearchLocationQuery) *redis.GeoSearchLocationCmd GeoRadius(ctx context.Context, key string, longitude, latitude float64, query *redis.GeoRadiusQuery) *redis.GeoLocationCmd + ZRem(ctx context.Context, key string, members ...interface{}) *redis.IntCmd +} + +// Remove drops a rider from the GEO set. +// +// ─── Why this has to exist ───────────────────────────────────────────────── +// +// A GEO set member never expires. Nothing removed a rider when they went off +// duty, so `milers:locations` accumulated every rider who had ever started a +// shift, frozen at their last reported position — for good. +// +// That is not merely untidy, because assignment asks for the TEN NEAREST +// members (geoMaxCount in internal/assignment). Riders who finished weeks ago, +// parked near the hub where most pickups originate, are geographically the +// closest members there are. They fill all ten slots, every one of them is +// then discarded by the GPS-freshness check, and the candidate pool collapses +// to whoever happened to survive — often a single rider. +// +// The balancing logic below that (leastLoaded, betterChoice) is correct and +// irrelevant: it can only balance across the pool it is handed. The symptom is +// every order in a city going to the same person. +// +// Called on end-duty. A failure is logged by the caller and not fatal: a rider +// left in the set is the status quo, not a regression. +func Remove(ctx context.Context, rdb Client, milerUserID int) error { + if rdb == nil { + return nil + } + return rdb.ZRem(ctx, Key, strconv.Itoa(milerUserID)).Err() } // legacyOnly flips to true the first time the server rejects GEOSEARCH as an diff --git a/internal/milergeo/milergeo_test.go b/internal/milergeo/milergeo_test.go index 8f9063c..27c9b2d 100644 --- a/internal/milergeo/milergeo_test.go +++ b/internal/milergeo/milergeo_test.go @@ -17,6 +17,19 @@ type fakeRedis struct { locs []redis.GeoLocation searchHits int radiusHits int + remErr error + removed []interface{} +} + +func (f *fakeRedis) ZRem(ctx context.Context, _ string, members ...interface{}) *redis.IntCmd { + f.removed = append(f.removed, members...) + cmd := redis.NewIntCmd(ctx) + if f.remErr != nil { + cmd.SetErr(f.remErr) + } else { + cmd.SetVal(int64(len(members))) + } + return cmd } func (f *fakeRedis) GeoSearchLocation(ctx context.Context, _ string, q *redis.GeoSearchLocationQuery) *redis.GeoSearchLocationCmd { @@ -118,3 +131,45 @@ func TestNilClientIsRefused(t *testing.T) { t.Fatalf("probe = %q", p) } } + +// A GEO member never expires, and nothing removed one when a rider went off +// duty — so milers:locations kept every rider who had ever started a shift, +// frozen where they last reported. Assignment takes the N NEAREST members, so +// those stale entries crowded out riders who were actually working and the +// candidate pool collapsed to whoever survived the freshness filter. +func TestRemoveDropsTheRiderFromTheSet(t *testing.T) { + f := &fakeRedis{} + if err := Remove(context.Background(), f, 412); err != nil { + t.Fatalf("Remove: %v", err) + } + if len(f.removed) != 1 || f.removed[0] != "412" { + t.Errorf("removed = %v, want [\"412\"]", f.removed) + } +} + +// The member is the rider's userid as a DECIMAL STRING — the same spelling +// GeoAdd writes and Search reads back. A mismatch here would remove nothing +// and report success. +func TestRemoveUsesTheSameMemberSpellingAsAdd(t *testing.T) { + f := &fakeRedis{} + _ = Remove(context.Background(), f, 7) + if f.removed[0] != "7" { + t.Errorf("member = %v, want \"7\" (decimal string, not an int)", f.removed[0]) + } +} + +// Best-effort at the call site: the rider is off duty either way, and the +// freshness check still excludes them. The error must reach the caller so it +// can be logged rather than swallowed here. +func TestRemoveReturnsTheError(t *testing.T) { + f := &fakeRedis{remErr: errors.New("redis down")} + if err := Remove(context.Background(), f, 1); err == nil { + t.Error("Remove swallowed the error") + } +} + +func TestRemoveOnNilClientIsANoOp(t *testing.T) { + if err := Remove(context.Background(), nil, 1); err != nil { + t.Errorf("Remove(nil) = %v, want nil", err) + } +} diff --git a/internal/prediction/calibration.go b/internal/prediction/calibration.go new file mode 100644 index 0000000..d36f6aa --- /dev/null +++ b/internal/prediction/calibration.go @@ -0,0 +1,272 @@ +package prediction + +import ( + "time" + + "doormile/constants" + "doormile/db" + "doormile/utils" + + "gorm.io/gorm" +) + +// The calibration refresh. +// +// Every delivered consignment whose rider was sequenced carries two numbers: +// what the Route Optimization API predicted (bookingassignments.etaminutes, +// written by internal/routing) and what actually happened (the Delivered row in +// consignmenthistory). The ratio between them, grouped and taken at p80, is the +// factor ETAMinutes multiplies by. +// +// Why p80 and not the mean: docs/prediction-plan.md §8 decision 3. An ETA is a +// promise, and a mean is late half the time. +// +// Why a view and not a join here: consignments has no bookingid column (hazard +// H4 in the plan). The consignment_booking view resolves the two paths — +// bookingdestinations for multi-destination pickups, the legacy +// pickupbookings.consignmentid for console and pre-fan-out rows — once, in +// migrate.go. Joining through consignments directly attaches features to the +// wrong parcel for every multi-destination booking, silently. + +// ETACalibration is one calibration cell as stored. Written only by Refresh, +// read at boot and after each refresh. +type ETACalibration struct { + Calibrationid int `gorm:"primaryKey;column:calibrationid;autoIncrement"` + Scope string `gorm:"column:scope;size:24;not null;index:idx_etacalibration_lookup,priority:1"` + Zone string `gorm:"column:zone;size:8;index:idx_etacalibration_lookup,priority:2"` + Hourbucket *int `gorm:"column:hourbucket;index:idx_etacalibration_lookup,priority:3"` + Weekday *int `gorm:"column:weekday;index:idx_etacalibration_lookup,priority:4"` + Factor float64 `gorm:"column:factor;not null"` + Handling float64 `gorm:"column:handling;not null;default:0"` + Samples int `gorm:"column:samples;not null"` + Refreshedat time.Time `gorm:"column:refreshedat;not null"` +} + +func (ETACalibration) TableName() string { return "etacalibration" } + +// row is one group from the refresh query. +type row struct { + Scope string + Zone string + Hourbucket *int + Weekday *int + Factor float64 + Samples int +} + +// Observed durations below this are almost certainly data errors — a Delivered +// event written in the same second as the consignment, or a backfill. Including +// them drags every factor toward zero. +const minObservedMinutes = 5 + +// And above this, the parcel sat for days for a reason that has nothing to do +// with road duration (held at hub, re-attempts, disputes). Calibrating a +// routed-duration multiplier on those teaches it the wrong thing. +const maxObservedMinutes = 48 * 60 + +// refreshSQL computes the p80 ratio of actual to routed duration, at three +// grains plus a global row, in one pass. +// +// The `actual` term uses consignmenthistory.createdat - consignments.createdat. +// Both come from CURRENT_TIMESTAMP defaults, so they share a tagging and their +// difference is correct regardless of hazard H1 — which is why this does not +// go near estimateddeliveryat, whose writes are not uniform +// (adminController.go:2738 uses time.Now(), elsewhere it is CURRENT_TIMESTAMP). +// +// EXTRACT(ISODOW) is 1..7 Monday-first, matching weekdayOf in eta.go. The hour +// bucket divides by 3, matching hourBucketSize. If either changes, both change. +// A booking can hold SEVERAL bookingassignments rows — Assigned, Rejected, +// Reassigned, Cancelled are all statuses it passes through. Joining them all +// would multiply one delivered parcel by its whole assignment history and pull +// the p80 toward whatever got reassigned most, so `assigned` picks exactly one +// row per booking: the most recently sequenced one, which is the routed ETA +// that was actually in force when the parcel was delivered. +// +// NULL::int / NULL::text are explicit because a UNION ALL resolves column types +// across its branches; an untyped NULL can be inferred as text and fail against +// the integer from the first branch — at runtime, against the real database, +// which is exactly where it is most expensive to discover. +const refreshSQL = ` +WITH assigned AS ( + SELECT DISTINCT ON (ba.bookingid) + ba.bookingid, + ba.etaminutes + FROM bookingassignments ba + WHERE ba.etaminutes > 0 + ORDER BY ba.bookingid, ba.sequencedat DESC NULLS LAST, ba.bookingassignmentid DESC +), +observed AS ( + SELECT left(c.deliverypincode, 3) AS zone, + (EXTRACT(HOUR FROM c.createdat)::int / ?) AS hourbucket, + EXTRACT(ISODOW FROM c.createdat)::int AS weekday, + a.etaminutes::double precision AS routed, + EXTRACT(EPOCH FROM (h.createdat - c.createdat)) / 60.0 AS actual + FROM consignments c + JOIN consignmenthistory h + ON h.consignmentid = c.consignmentid + AND h.eventstatus = ? + JOIN consignment_booking cb + ON cb.consignmentid = c.consignmentid + JOIN assigned a + ON a.bookingid = cb.bookingid + WHERE c.deliverypincode IS NOT NULL + AND length(c.deliverypincode) >= 3 +), +clean AS ( + SELECT * FROM observed + WHERE actual BETWEEN ? AND ? + AND routed > 0 +) +SELECT 'zone_hour_weekday'::text AS scope, zone, hourbucket, weekday, + percentile_cont(0.8) WITHIN GROUP (ORDER BY actual / routed) AS factor, + count(*)::int AS samples +FROM clean GROUP BY zone, hourbucket, weekday +UNION ALL +SELECT 'zone_weekday'::text, zone, NULL::int, weekday, + percentile_cont(0.8) WITHIN GROUP (ORDER BY actual / routed), count(*)::int +FROM clean GROUP BY zone, weekday +UNION ALL +SELECT 'zone'::text, zone, NULL::int, NULL::int, + percentile_cont(0.8) WITHIN GROUP (ORDER BY actual / routed), count(*)::int +FROM clean GROUP BY zone +UNION ALL +SELECT 'global'::text, ''::text, NULL::int, NULL::int, + percentile_cont(0.8) WITHIN GROUP (ORDER BY actual / routed), count(*)::int +FROM clean +` + +// Refresh recomputes the calibration from history, replaces the stored table, +// and swaps the in-memory snapshot. +// +// A failure leaves the previous calibration in place — a refresh that cannot +// run is not a reason to stop answering with the last good factors. If they go +// stale past staleAfter, ETAMinutes stops trusting them on its own. +func Refresh(gdb *gorm.DB) error { + if gdb == nil { + return nil + } + + var rows []row + if err := gdb.Raw(refreshSQL, + hourBucketSize, constants.ConsignmentDelivered, minObservedMinutes, maxObservedMinutes, + ).Scan(&rows).Error; err != nil { + return err + } + + kept := make([]ETACalibration, 0, len(rows)) + now := utils.DBNow() + for _, r := range rows { + if r.Samples < minSamples { + continue // a p80 over fewer than minSamples is noise + } + if r.Factor < minFactor || r.Factor > maxFactor { + // Out of bounds is a signal about the data, not a usable factor. + // Log it once here rather than discovering it per request. + utils.Warn("prediction: discarding out-of-bounds calibration factor", + "scope", r.Scope, "zone", r.Zone, "factor", r.Factor, "samples", r.Samples) + continue + } + kept = append(kept, ETACalibration{ + Scope: r.Scope, + Zone: r.Zone, + Hourbucket: r.Hourbucket, + Weekday: r.Weekday, + Factor: r.Factor, + Samples: r.Samples, + Refreshedat: now, + }) + } + + if len(kept) == 0 { + // No cell cleared the floor. Expected until routing is on and history + // accumulates; ETAMinutes keeps returning false and the promise tables + // keep answering. + utils.Info("prediction: no calibration cells met the sample floor", + "groups_considered", len(rows), "min_samples", minSamples) + return nil + } + + // Replace wholesale in one transaction: a partial table would serve a mix + // of old and new factors for the same zone. + if err := gdb.Transaction(func(tx *gorm.DB) error { + if err := tx.Exec(`DELETE FROM etacalibration`).Error; err != nil { + return err + } + return tx.CreateInBatches(&kept, 200).Error + }); err != nil { + return err + } + + Load(kept) + utils.Info("prediction: calibration refreshed", "cells", len(kept)) + return nil +} + +// Load swaps the in-memory snapshot. Exported so boot can populate from the +// stored table without recomputing, and so tests can install a known +// calibration without a database. +// +// builtAt is the NEWEST Refreshedat among the rows, not time.Now(): a restart +// that loads month-old rows from the store must still look month-old to the +// staleness guard in ETAMinutes. Taking the load time here would silently +// re-arm a stale calibration on every deploy. A row with no Refreshedat (a test +// fixture) is treated as fresh. +// Refreshedat is stored through utils.DBNow, which writes IST wall-clock digits +// labelled UTC. Reading it back as an instant is off by 5h30m, so it goes +// through utils.IST first — the same correction utils/epoch.go exists for. +// Without it a fresh calibration reads as 5.5 hours old, which is the very +// hazard docs/prediction-plan.md calls H1. +func Load(rows []ETACalibration) { + t := &table{cells: make(map[string]cell, len(rows))} + for _, r := range rows { + if r.Refreshedat.IsZero() { + continue + } + if at := utils.IST(r.Refreshedat); at.After(t.builtAt) { + t.builtAt = at + } + } + if t.builtAt.IsZero() { + t.builtAt = time.Now() // a fixture with no Refreshedat counts as fresh + } + for _, r := range rows { + c := cell{factor: r.Factor, handling: r.Handling, samples: r.Samples} + switch r.Scope { + case "global": + t.global, t.hasGlobal = c, true + case "zone": + t.cells[keyZone(r.Zone)] = c + case "zone_weekday": + if r.Weekday != nil { + t.cells[keyZoneWeekday(r.Zone, *r.Weekday)] = c + } + case "zone_hour_weekday": + if r.Weekday != nil && r.Hourbucket != nil { + t.cells[keyZoneHourWeekday(r.Zone, *r.Hourbucket, *r.Weekday)] = c + } + } + } + current.store(t) +} + +// Reset clears the in-memory calibration. Tests only — it makes ETAMinutes +// return false, which is the state a fresh process is in before boot loads. +func Reset() { current.store(nil) } + +// LoadFromDB populates the snapshot from the stored table at boot, so a restart +// does not wait for the next refresh to start answering. +func LoadFromDB() { + if db.DB == nil { + return + } + var rows []ETACalibration + if err := db.DB.Find(&rows).Error; err != nil { + utils.Warn("prediction: could not load stored calibration", "error", err) + return + } + if len(rows) == 0 { + return + } + Load(rows) + utils.Info("prediction: calibration loaded from store", "cells", len(rows)) +} diff --git a/internal/prediction/eta.go b/internal/prediction/eta.go new file mode 100644 index 0000000..c605d8b --- /dev/null +++ b/internal/prediction/eta.go @@ -0,0 +1,277 @@ +// Package prediction turns Doormile's own past predictions into better ones. +// +// Doormile already promises a delivery time, in two places and both of them +// fixed tables: controllers/adminController.go uses service-type constants +// (Normal 24h, Fast 12h, Superfast 6h) and controllers/cxPickupFanout.go uses +// the destination district's `promise` column (same-day/next-day/2-day/3-day). +// Neither looks at a single delivery that actually happened. +// +// This package does. The Route Optimization API already returns a road-network +// duration per stop and internal/routing already stores it on +// bookingassignments.etaminutes. Pairing that stored prediction with the +// Delivered event in consignmenthistory gives a predicted-vs-actual series the +// system produced itself, and a grouped p80 over it is a calibration factor: +// +// eta = routed_minutes × factor[zone, hour_bucket, weekday] + handling[zone] +// +// That is a median, not a model. No training, no inference server, no new +// runtime — a table refreshed nightly and a map lookup on the booking path. +// +// ─── The floor rule ──────────────────────────────────────────────────────── +// +// ETAMinutes returns (0, false) whenever it lacks the evidence to do better, +// and every caller keeps its existing rule for that case. So: +// +// - no routed duration (ROUTE_OPTIMIZER_URL unset, or a single-stop rider) → false +// - no calibration cell with enough samples → false +// - calibration never refreshed, or the refresh failed → false +// - a factor outside sane bounds → false +// +// Today, in the cluster, ROUTE_OPTIMIZER_URL is not set (see Phase 7 Track A1), +// so this returns false for every booking and the promise tables answer exactly +// as they do now. Switch routing on, let a few weeks of deliveries land, and it +// starts answering — with no code change and no deploy. Today's behaviour is +// the floor; this can only raise it. +// +// ─── p80, not the mean ───────────────────────────────────────────────────── +// +// The calibration is the 80th percentile of observed overrun, not the average. +// An ETA shown to a customer is a promise — "arrives by" — and a mean is late +// half the time. docs/prediction-plan.md §8 decision 3. +package prediction + +import ( + "strconv" + "strings" + "sync" + "time" + + "doormile/utils" +) + +const ( + // minSamples is the floor for trusting one calibration cell. Below it the + // p80 is noise and the lookup falls back to a coarser key. + minSamples = 20 + + // A factor outside these bounds means the calibration is wrong, not that + // deliveries are 10× their routed duration. Refuse rather than serve it: + // an absurd ETA is worse than today's flat constant. + minFactor = 0.5 + maxFactor = 5.0 + + // maxHandlingMinutes caps the additive hub term for the same reason. + maxHandlingMinutes = 240 + + // staleAfter is how long a calibration stays usable without a refresh. The + // sweeper runs far more often than this; exceeding it means the refresh has + // been failing silently, and a month-old factor should not keep answering. + staleAfter = 72 * time.Hour + + // hourBucketSize groups the day into 8 three-hour buckets. Finer buckets + // split the samples too thin to clear minSamples on real volume. + hourBucketSize = 3 +) + +// Input is everything the estimate needs. Built by the caller from the booking +// it already has in hand; this package never queries on the request path. +type Input struct { + // RoutedMinutes is bookingassignments.etaminutes — the Route Optimization + // API's road-network duration. Zero means unknown, which is the common case + // today and the whole reason for the floor rule. + RoutedMinutes int + + // DeliveryPincode keys the zone. Only its first three digits are used, the + // same grain the hub console filters on (pickuppincode LIKE '641%'). + DeliveryPincode string + + // At is when the estimate is being made. Interpreted through utils.IST, + // because this database stores IST wall-clock digits and a raw .Hour() on a + // value tagged UTC is off by 5h30m — the defect utils/epoch.go documents. + At time.Time +} + +// Result is a calibrated estimate and the cell that produced it. Source is for +// logging and for the console to say where a number came from; nothing branches +// on it. +type Result struct { + Minutes int + Source string // "zone_hour_weekday", "zone_weekday", "zone", "global" + Samples int +} + +// cell is one calibration row, in memory. +type cell struct { + factor float64 + handling float64 + samples int +} + +// table is an immutable calibration snapshot. Replaced wholesale by the +// refresh; readers never see a half-updated map. +type table struct { + cells map[string]cell + global cell + hasGlobal bool + builtAt time.Time +} + +var ( + current atomic[*table] +) + +// atomic is a tiny generic holder. sync/atomic.Pointer would do, but this keeps +// the zero value useful (an unset calibration reads as nil, which ETAMinutes +// treats as "no evidence") without an init func. +type atomic[T any] struct { + mu sync.RWMutex + v T +} + +func (a *atomic[T]) load() T { + a.mu.RLock() + defer a.mu.RUnlock() + return a.v +} + +func (a *atomic[T]) store(v T) { + a.mu.Lock() + a.v = v + a.mu.Unlock() +} + +// zoneOf is the first three digits of a pincode — the same grain the hub +// console scopes on. An empty or short pincode has no zone, which the lookup +// treats as "global only". +func zoneOf(pincode string) string { + p := strings.TrimSpace(pincode) + if len(p) < 3 { + return "" + } + return p[:3] +} + +// hourBucket is the 3-hour block of the IST day, 0..7. +func hourBucket(t time.Time) int { + return utils.IST(t).Hour() / hourBucketSize +} + +// weekdayOf is the ISO weekday in IST, 1 (Monday) to 7 (Sunday) — matching +// Postgres's EXTRACT(ISODOW) so the Go lookup and the refresh SQL agree. +func weekdayOf(t time.Time) int { + d := int(utils.IST(t).Weekday()) + if d == 0 { + return 7 // Go's Sunday is 0; ISO's is 7 + } + return d +} + +// keyZoneHourWeekday, keyZoneWeekday and keyZone are the three progressively +// coarser lookups. Distinct prefixes so a zone can never collide with a +// weekday-qualified key. +func keyZoneHourWeekday(zone string, bucket, weekday int) string { + return "zhw:" + zone + ":" + strconv.Itoa(bucket) + ":" + strconv.Itoa(weekday) +} + +func keyZoneWeekday(zone string, weekday int) string { + return "zw:" + zone + ":" + strconv.Itoa(weekday) +} + +func keyZone(zone string) string { + return "z:" + zone +} + +// ETAMinutes returns a calibrated door-to-door estimate in minutes, and whether +// it is trustworthy. +// +// False means the caller must use whatever it does today — its service-type +// constants or the district promise table. A false is not an error and is not +// logged per call: it is the expected answer until routing is switched on and +// history accumulates. +func ETAMinutes(in Input) (Result, bool) { + if in.RoutedMinutes <= 0 { + // No road-network duration to calibrate against. The dominant case + // today: ROUTE_OPTIMIZER_URL is unset in the cluster, and even with it + // set, internal/routing skips riders with fewer than two stops. + return Result{}, false + } + + t := current.load() + if t == nil || len(t.cells) == 0 && !t.hasGlobal { + return Result{}, false + } + if !t.builtAt.IsZero() && time.Since(t.builtAt) > staleAfter { + // A refresh has been failing for days. Fall back rather than serve a + // factor that predates whatever changed. + return Result{}, false + } + + zone := zoneOf(in.DeliveryPincode) + bucket, weekday := hourBucket(in.At), weekdayOf(in.At) + + type candidate struct { + key string + source string + } + candidates := []candidate{} + if zone != "" { + candidates = append(candidates, + candidate{keyZoneHourWeekday(zone, bucket, weekday), "zone_hour_weekday"}, + candidate{keyZoneWeekday(zone, weekday), "zone_weekday"}, + candidate{keyZone(zone), "zone"}, + ) + } + + for _, c := range candidates { + if cl, ok := t.cells[c.key]; ok && cl.samples >= minSamples { + if m, ok := apply(in.RoutedMinutes, cl); ok { + return Result{Minutes: m, Source: c.source, Samples: cl.samples}, true + } + } + } + + if t.hasGlobal && t.global.samples >= minSamples { + if m, ok := apply(in.RoutedMinutes, t.global); ok { + return Result{Minutes: m, Source: "global", Samples: t.global.samples}, true + } + } + + return Result{}, false +} + +// apply is the estimate itself, with the bounds check that keeps a bad +// calibration from producing an absurd promise. +func apply(routedMinutes int, c cell) (int, bool) { + if c.factor < minFactor || c.factor > maxFactor { + return 0, false + } + if c.handling < 0 || c.handling > maxHandlingMinutes { + return 0, false + } + m := float64(routedMinutes)*c.factor + c.handling + if m <= 0 { + return 0, false + } + return int(m + 0.5), true +} + +// ETAAt is ETAMinutes as an absolute time, for the callers that store a +// timestamp rather than a duration. The returned time is in the same shape the +// caller's `from` was, so it round-trips into the database unchanged. +func ETAAt(in Input, from time.Time) (time.Time, Result, bool) { + r, ok := ETAMinutes(in) + if !ok { + return time.Time{}, Result{}, false + } + return from.Add(time.Duration(r.Minutes) * time.Minute), r, true +} + +// Loaded reports whether a usable calibration is in memory. For +// GET /admin/ai/status and the readiness note; not used on the booking path. +func Loaded() (bool, time.Time, int) { + t := current.load() + if t == nil { + return false, time.Time{}, 0 + } + return len(t.cells) > 0 || t.hasGlobal, t.builtAt, len(t.cells) +} diff --git a/internal/prediction/eta_test.go b/internal/prediction/eta_test.go new file mode 100644 index 0000000..22600ef --- /dev/null +++ b/internal/prediction/eta_test.go @@ -0,0 +1,284 @@ +package prediction + +import ( + "testing" + "time" + + "doormile/utils" +) + +func ptr(n int) *int { return &n } + +// A calibration that is deliberately coarse-to-fine, so the fallback ladder is +// observable: the zone_hour_weekday cell has a different factor from the +// zone_weekday one, which differs from zone, which differs from global. +func fixture() []ETACalibration { + return []ETACalibration{ + {Scope: "zone_hour_weekday", Zone: "641", Hourbucket: ptr(3), Weekday: ptr(1), Factor: 1.5, Samples: 100}, + {Scope: "zone_weekday", Zone: "641", Weekday: ptr(1), Factor: 2.0, Samples: 100}, + {Scope: "zone", Zone: "641", Factor: 2.5, Samples: 100}, + {Scope: "global", Factor: 3.0, Samples: 100}, + } +} + +// 2026-10-05 is a Monday. 10:00 IST is hour bucket 3 (10/3). The database +// stores IST digits labelled UTC, so the fixture time is built that way on +// purpose — it is the shape a real column read produces. +func mondayTenAM() time.Time { + return time.Date(2026, 10, 5, 10, 0, 0, 0, time.UTC) +} + +// ─── The floor rule: these are the cases that must return false ──────────── + +func TestNoRoutedDurationFallsBack(t *testing.T) { + Load(fixture()) + defer Reset() + + // The dominant case in production today: ROUTE_OPTIMIZER_URL is unset, so + // internal/routing never runs and etaminutes is 0. + if _, ok := ETAMinutes(Input{RoutedMinutes: 0, DeliveryPincode: "641001", At: mondayTenAM()}); ok { + t.Fatal("ETAMinutes answered with no routed duration; the promise table must keep answering") + } + if _, ok := ETAMinutes(Input{RoutedMinutes: -5, DeliveryPincode: "641001", At: mondayTenAM()}); ok { + t.Fatal("ETAMinutes answered on a negative routed duration") + } +} + +func TestNoCalibrationFallsBack(t *testing.T) { + Reset() + if _, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}); ok { + t.Fatal("ETAMinutes answered with no calibration loaded") + } +} + +func TestBelowSampleFloorFallsBack(t *testing.T) { + Load([]ETACalibration{ + {Scope: "zone", Zone: "641", Factor: 2.0, Samples: minSamples - 1}, + }) + defer Reset() + + if _, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}); ok { + t.Fatalf("ETAMinutes trusted a cell with %d samples (floor is %d)", minSamples-1, minSamples) + } +} + +func TestOutOfBoundsFactorFallsBack(t *testing.T) { + for _, f := range []float64{minFactor - 0.1, maxFactor + 0.1, 0, -1} { + Load([]ETACalibration{{Scope: "zone", Zone: "641", Factor: f, Samples: 100}}) + if _, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}); ok { + t.Errorf("ETAMinutes served an out-of-bounds factor %v; an absurd ETA is worse than a flat constant", f) + } + Reset() + } +} + +// This is also the H1 regression test, verified by mutation: Refreshedat is +// written through utils.DBNow (IST wall-clock digits labelled UTC), so reading +// it as a raw instant makes it appear 5h30m NEWER than it is. A calibration +// 73h old then measures as 67.5h and slips under the 72h staleAfter bound. +// Removing the utils.IST call in Load fails exactly this test. +// +// The margin matters: with staleAfter at 72h, the 5.5h error is a 7.6% window +// in which a stale calibration keeps answering. Narrow staleAfter and the bug +// gets proportionally worse, which is why the correction belongs in Load rather +// than in a wider bound here. +func TestStaleCalibrationFallsBack(t *testing.T) { + old := utils.DBNow().Add(-(staleAfter + time.Hour)) + Load([]ETACalibration{ + {Scope: "zone", Zone: "641", Factor: 2.0, Samples: 100, Refreshedat: old}, + }) + defer Reset() + + if _, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}); ok { + t.Fatal("ETAMinutes trusted a calibration older than staleAfter; refreshes have been failing") + } +} + +// The companion case: a just-refreshed calibration must answer. Note this one +// passes with or without the IST correction (the raw read errs toward "newer", +// not "older"), so it documents intent rather than guarding the bug — the guard +// above is what fails on mutation. +func TestFreshCalibrationIsNotMisreadAsStale(t *testing.T) { + Load([]ETACalibration{ + {Scope: "zone", Zone: "641", Factor: 2.0, Samples: 100, Refreshedat: utils.DBNow()}, + }) + defer Reset() + + if _, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}); !ok { + t.Fatal("a just-refreshed calibration was treated as stale — the DBNow/IST correction in Load is wrong") + } +} + +// Regression: loading month-old rows from the store at boot must stay stale. +// Stamping builtAt with time.Now() here would re-arm a dead calibration on +// every deploy. +func TestBootLoadDoesNotRearmStaleRows(t *testing.T) { + old := utils.DBNow().Add(-(staleAfter + 24*time.Hour)) + Load([]ETACalibration{ + {Scope: "zone", Zone: "641", Factor: 2.0, Samples: 100, Refreshedat: old}, + {Scope: "global", Factor: 2.2, Samples: 100, Refreshedat: old}, + }) + defer Reset() + + if _, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}); ok { + t.Fatal("a restart re-armed a stale stored calibration") + } +} + +// ─── The ladder: most specific cell wins, then progressively coarser ─────── + +func TestMostSpecificCellWins(t *testing.T) { + Load(fixture()) + defer Reset() + + r, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}) + if !ok { + t.Fatal("expected an estimate") + } + if r.Source != "zone_hour_weekday" { + t.Errorf("source = %q, want zone_hour_weekday", r.Source) + } + if r.Minutes != 60 { // 40 × 1.5 + t.Errorf("minutes = %d, want 60 (40 × 1.5)", r.Minutes) + } +} + +func TestFallsToZoneWeekdayThenZoneThenGlobal(t *testing.T) { + full := fixture() + defer Reset() + + // Drop the finest cell: 13:00 IST is bucket 4, for which there is no + // zone_hour_weekday row, so zone_weekday answers. + Load(full) + r, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", + At: time.Date(2026, 10, 5, 13, 0, 0, 0, time.UTC)}) + if !ok || r.Source != "zone_weekday" || r.Minutes != 80 { // 40 × 2.0 + t.Errorf("got %+v ok=%v, want zone_weekday 80", r, ok) + } + + // A Tuesday has no weekday cell for this zone, so zone answers. + r, ok = ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", + At: time.Date(2026, 10, 6, 13, 0, 0, 0, time.UTC)}) + if !ok || r.Source != "zone" || r.Minutes != 100 { // 40 × 2.5 + t.Errorf("got %+v ok=%v, want zone 100", r, ok) + } + + // An unknown zone falls all the way to global. + r, ok = ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "500081", At: mondayTenAM()}) + if !ok || r.Source != "global" || r.Minutes != 120 { // 40 × 3.0 + t.Errorf("got %+v ok=%v, want global 120", r, ok) + } +} + +func TestShortOrEmptyPincodeUsesGlobalOnly(t *testing.T) { + Load(fixture()) + defer Reset() + + for _, p := range []string{"", "6", "64", " "} { + r, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: p, At: mondayTenAM()}) + if !ok || r.Source != "global" { + t.Errorf("pincode %q: got %+v ok=%v, want global", p, r, ok) + } + } +} + +// ─── Bucket arithmetic must match the refresh SQL, or the lookup misses ──── + +func TestHourBucketMatchesRefreshSQL(t *testing.T) { + // refreshSQL does EXTRACT(HOUR FROM createdat)::int / hourBucketSize. + // hourBucket must agree exactly, or every finest-grain lookup misses. + for hour := 0; hour < 24; hour++ { + got := hourBucket(time.Date(2026, 10, 5, hour, 30, 0, 0, time.UTC)) + if want := hour / hourBucketSize; got != want { + t.Errorf("hour %d: bucket = %d, want %d", hour, got, want) + } + } +} + +func TestWeekdayMatchesPostgresISODOW(t *testing.T) { + // EXTRACT(ISODOW) is Monday=1 … Sunday=7. Go's time.Weekday is Sunday=0. + // 2026-10-05 is a Monday. + want := map[int]int{5: 1, 6: 2, 7: 3, 8: 4, 9: 5, 10: 6, 11: 7} + for day, w := range want { + got := weekdayOf(time.Date(2026, 10, day, 12, 0, 0, 0, time.UTC)) + if got != w { + t.Errorf("2026-10-%02d: weekday = %d, want %d (ISODOW)", day, got, w) + } + } +} + +func TestZoneIsPincodePrefix(t *testing.T) { + cases := map[string]string{ + "641001": "641", "641 ": "641", "500081": "500", + "": "", "64": "", " ": "", + } + for in, want := range cases { + if got := zoneOf(in); got != want { + t.Errorf("zoneOf(%q) = %q, want %q", in, got, want) + } + } +} + +// ─── ETAAt and handling term ─────────────────────────────────────────────── + +func TestETAAtAddsToTheGivenTime(t *testing.T) { + Load(fixture()) + defer Reset() + + from := mondayTenAM() + at, r, ok := ETAAt(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: from}, from) + if !ok { + t.Fatal("expected an estimate") + } + if want := from.Add(60 * time.Minute); !at.Equal(want) { + t.Errorf("ETAAt = %v, want %v", at, want) + } + // The returned time keeps the caller's location, so it round-trips into the + // database in the same shape it came out. + if at.Location() != from.Location() { + t.Errorf("ETAAt changed the location from %v to %v", from.Location(), at.Location()) + } + if r.Minutes != 60 { + t.Errorf("result minutes = %d, want 60", r.Minutes) + } +} + +func TestETAAtFallsBackWithoutCalibration(t *testing.T) { + Reset() + from := mondayTenAM() + if _, _, ok := ETAAt(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: from}, from); ok { + t.Fatal("ETAAt answered with no calibration") + } +} + +func TestHandlingTermIsAddedAndBounded(t *testing.T) { + Load([]ETACalibration{ + {Scope: "zone", Zone: "641", Factor: 2.0, Handling: 15, Samples: 100}, + }) + r, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}) + if !ok || r.Minutes != 95 { // 40 × 2.0 + 15 + t.Errorf("got %+v ok=%v, want 95", r, ok) + } + Reset() + + Load([]ETACalibration{ + {Scope: "zone", Zone: "641", Factor: 2.0, Handling: maxHandlingMinutes + 1, Samples: 100}, + }) + if _, ok := ETAMinutes(Input{RoutedMinutes: 40, DeliveryPincode: "641001", At: mondayTenAM()}); ok { + t.Error("served a handling term above the cap") + } + Reset() +} + +func TestLoadedReportsState(t *testing.T) { + Reset() + if ok, _, _ := Loaded(); ok { + t.Error("Loaded() true after Reset") + } + Load(fixture()) + defer Reset() + ok, _, cells := Loaded() + if !ok || cells != 3 { // 3 keyed cells; global is held separately + t.Errorf("Loaded() = %v, cells %d; want true, 3", ok, cells) + } +} diff --git a/internal/prediction/refine.go b/internal/prediction/refine.go new file mode 100644 index 0000000..d4340a2 --- /dev/null +++ b/internal/prediction/refine.go @@ -0,0 +1,185 @@ +package prediction + +import ( + "time" + + "doormile/constants" + "doormile/models" + "doormile/utils" + + "gorm.io/gorm" +) + +// Refining a promise after the customer has already been given one. +// +// ─── Why this cannot happen at booking-create time ───────────────────────── +// +// The obvious place to use a calibrated ETA is where the promise is first +// written — controllers/adminController.go:2738 and +// controllers/cxPickupFanout.go:224. It does not work there, and not because +// the calibration is cold: at that moment there is structurally no routed +// duration to calibrate. The order of events is +// +// booking created -> estimateddeliveryat written from the promise table +// -> rider assigned +// -> internal/routing sequences -> etaminutes written +// +// so RoutedMinutes is 0 on every create and ETAMinutes would return false +// forever. Wiring it there would be dead code. +// +// The routed duration first exists at sequencing. So refining the promise means +// revising a number the customer may already have seen. +// +// ─── What this does and deliberately does not do ─────────────────────────── +// +// It updates consignments.estimateddeliveryat — the "arrives by" the customer +// is shown — and never touches sladueat. That split is the whole design: +// +// estimateddeliveryat our best current belief. Allowed to improve. +// sladueat the commitment made at booking. Must not move, or it +// stops being a commitment and every breach can be +// explained away by moving the target. +// +// It also only ever moves the estimate when the calibration is trustworthy +// (ETAMinutes true), so with ROUTE_OPTIMIZER_URL unset — the state today — +// this function reads some rows and writes nothing. +// +// A customer watching the tracking page will see the time change. Whether that +// should also emit a stage event is an open product question +// (docs/prediction-plan.md §3a); this writes the column and does not notify, +// because inventing a notification is the more consequential of the two +// choices to get wrong. + +// RefinedETA is one estimate this pass revised, for logging. +type RefinedETA struct { + Consignmentid int + Was *time.Time + Now time.Time + Source string +} + +// refineRow is a consignment awaiting delivery, joined to the routed duration +// of its booking's latest sequenced assignment. +// +// The join goes through consignment_booking because consignments has no +// bookingid column, and the legacy pickupbookings.consignmentid link names only +// the FIRST order of a multi-destination pickup — hazard H4. DISTINCT ON picks +// one assignment per booking: a booking passes through several +// (Assigned/Rejected/Reassigned) and the newest sequenced one is the routed ETA +// actually in force. +type refineRow struct { + Consignmentid int + Deliverypincode string + Createdat time.Time + Estimateddeliveryat *time.Time + Etaminutes int + Sequencedat *time.Time +} + +const refineSQL = ` +WITH assigned AS ( + SELECT DISTINCT ON (ba.bookingid) + ba.bookingid, ba.etaminutes, ba.sequencedat + FROM bookingassignments ba + WHERE ba.etaminutes > 0 + ORDER BY ba.bookingid, ba.sequencedat DESC NULLS LAST, ba.bookingassignmentid DESC +) +SELECT c.consignmentid, + COALESCE(c.deliverypincode, '') AS deliverypincode, + c.createdat, + c.estimateddeliveryat, + a.etaminutes, + a.sequencedat +FROM consignments c +JOIN consignment_booking cb ON cb.consignmentid = c.consignmentid +JOIN assigned a ON a.bookingid = cb.bookingid +WHERE c.status NOT IN ? + AND c.deletedat IS NULL +ORDER BY a.sequencedat DESC NULLS LAST +LIMIT ? +` + +// refineBatch caps one pass. The sweeper runs often enough that a backlog +// clears over a few ticks, and an unbounded UPDATE loop on a hot table is the +// kind of thing that shows up as a latency spike somewhere unrelated. +const refineBatch = 300 + +// terminal statuses — a parcel that has arrived, come back, or been written off +// has nothing left to estimate. +func terminalStatuses() []string { + return []string{ + constants.ConsignmentDelivered, + "Returned_to_Sender", + "Missing", + "Damaged", + } +} + +// RefineInFlight recomputes estimateddeliveryat for consignments that are still +// moving and whose rider has a routed duration. Returns what it changed. +// +// Safe to call when nothing is calibrated: ETAMinutes returns false for every +// row and this writes nothing. +func RefineInFlight(gdb *gorm.DB, now time.Time) ([]RefinedETA, error) { + if gdb == nil { + return nil, nil + } + if ok, _, _ := Loaded(); !ok { + return nil, nil // nothing to refine with; the promise tables stand + } + + var rows []refineRow + if err := gdb.Raw(refineSQL, terminalStatuses(), refineBatch).Scan(&rows).Error; err != nil { + return nil, err + } + + changed := make([]RefinedETA, 0, len(rows)) + for _, r := range rows { + // The estimate is anchored on when sequencing happened, not on the + // consignment's creation: the routed duration describes the journey + // from the rider's current position onward. + anchor := r.Createdat + if r.Sequencedat != nil { + anchor = *r.Sequencedat + } + + eta, res, ok := ETAAt(Input{ + RoutedMinutes: r.Etaminutes, + DeliveryPincode: r.Deliverypincode, + At: anchor, + }, anchor) + if !ok { + continue + } + + // Skip a no-op write. Without this every pass rewrites every in-flight + // row with the same value, which churns WAL and makes the log useless + // for seeing what actually moved. + if r.Estimateddeliveryat != nil && withinAMinute(*r.Estimateddeliveryat, eta) { + continue + } + + if err := gdb.Model(&models.Consignment{}). + Where("consignmentid = ?", r.Consignmentid). + Update("estimateddeliveryat", eta).Error; err != nil { + utils.Warn("prediction: could not refine ETA", + "consignmentid", r.Consignmentid, "error", err) + continue + } + changed = append(changed, RefinedETA{ + Consignmentid: r.Consignmentid, + Was: r.Estimateddeliveryat, + Now: eta, + Source: res.Source, + }) + } + return changed, nil +} + +func withinAMinute(a, b time.Time) bool { + d := a.Sub(b) + if d < 0 { + d = -d + } + return d < time.Minute +} diff --git a/internal/prediction/sweeper.go b/internal/prediction/sweeper.go new file mode 100644 index 0000000..fb3e3e1 --- /dev/null +++ b/internal/prediction/sweeper.go @@ -0,0 +1,130 @@ +package prediction + +import ( + "context" + "os" + "strconv" + "strings" + "time" + + "doormile/db" + "doormile/utils" +) + +// The calibration sweeper. +// +// Deliberately the same shape as internal/assignment/sweeper.go: a ticker, a +// recover() per tick so one bad run cannot take the process down, and a Redis +// lock that expires before the next tick so only one replica of three does the +// work. Without Redis every replica refreshes, which is wasteful but correct — +// Refresh replaces the table in one transaction, so concurrent runs converge +// rather than interleave. +// +// This is a nightly job by default. The calibration is a p80 over weeks of +// deliveries; recomputing it more often costs a scan and changes nothing. + +const ( + defaultCalibrationSweepSeconds = 6 * 60 * 60 // 6h + calibrationLockKey = "prediction:calibration:lock" + + // Below this a "sweep" is a scan loop against the whole delivery history. + minCalibrationSweepSeconds = 600 +) + +// calibrationInterval: PREDICTION_CALIBRATION_SECONDS, default 6h; 0 turns the +// sweeper off and leaves whatever is in the store. Read once at start, matching +// how assignment's sweepInterval behaves. +func calibrationInterval() time.Duration { + if v := strings.TrimSpace(os.Getenv("PREDICTION_CALIBRATION_SECONDS")); v != "" { + if n, err := strconv.Atoi(v); err == nil && n >= 0 { + if n > 0 && n < minCalibrationSweepSeconds { + n = minCalibrationSweepSeconds + } + return time.Duration(n) * time.Second + } + utils.Warn("PREDICTION_CALIBRATION_SECONDS is not a non-negative integer, using the default", + "value", v, "default_seconds", defaultCalibrationSweepSeconds) + } + return defaultCalibrationSweepSeconds * time.Second +} + +// StartCalibrationSweeper loads whatever calibration is stored, then refreshes +// it on a timer. Call once at boot, in a goroutine. +// +// The load happens even when the sweeper is disabled: a stored calibration from +// a previous deploy is still the best available answer, and ETAMinutes will +// stop trusting it on its own once it passes staleAfter. +func StartCalibrationSweeper() { + LoadFromDB() + + interval := calibrationInterval() + if interval == 0 { + utils.Info("CalibrationSweeper: disabled (PREDICTION_CALIBRATION_SECONDS=0)") + return + } + utils.Info("CalibrationSweeper: started", "interval", interval.String()) + + // One refresh shortly after boot rather than waiting a full interval, so a + // newly deployed replica is not serving a day-old calibration for six + // hours. Offset so three replicas do not all wake together. + time.Sleep(90 * time.Second) + refreshOnce(interval) + + ticker := time.NewTicker(interval) + defer ticker.Stop() + for range ticker.C { + refreshOnce(interval) + } +} + +// refreshOnce makes one refresh attempt. Only one replica at a time (a Redis +// lock that expires before the next tick). +func refreshOnce(interval time.Duration) { + defer func() { + if r := recover(); r != nil { + utils.Error("CalibrationSweeper: panic recovered", "error", r) + } + }() + if db.DB == nil { + return + } + if db.Rdb != nil { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + ttl := interval - 60*time.Second + if ttl <= 0 { + ttl = interval / 2 + } + got, err := db.Rdb.SetNX(ctx, calibrationLockKey, "1", ttl).Result() + cancel() + if err == nil && !got { + return // another replica has this refresh + } + } + + started := time.Now() + if err := Refresh(db.DB); err != nil { + // A failed refresh leaves the previous calibration serving. That is the + // intended behaviour: the last good factors beat falling back to flat + // constants, and staleAfter puts a bound on how long that can last. + utils.Error("CalibrationSweeper: refresh failed", "error", err, + "took_ms", time.Since(started).Milliseconds()) + return + } + ok, builtAt, cells := Loaded() + utils.Info("CalibrationSweeper: refresh complete", + "loaded", ok, "cells", cells, "built_at", builtAt, + "took_ms", time.Since(started).Milliseconds()) + + // Apply the fresh calibration to parcels still in flight. Writes + // estimateddeliveryat only — never sladueat, which is the commitment made + // at booking (see internal/prediction/refine.go). A no-op while nothing is + // calibrated. + refined, err := RefineInFlight(db.DB, time.Now()) + if err != nil { + utils.Error("CalibrationSweeper: ETA refine failed", "error", err) + return + } + if len(refined) > 0 { + utils.Info("CalibrationSweeper: refined in-flight ETAs", "count", len(refined)) + } +} diff --git a/main.go b/main.go index f186ac1..d30646f 100644 --- a/main.go +++ b/main.go @@ -13,11 +13,13 @@ import ( "doormile/config" "doormile/controllers" "doormile/db" + "doormile/internal/ai/outcomes" "doormile/internal/ai/playground" "doormile/internal/ai/telemetry" "doormile/internal/assignment" "doormile/internal/milergeo" "doormile/internal/notify" + "doormile/internal/prediction" "doormile/internal/routing" "doormile/internal/sms" "doormile/internal/worker" @@ -242,9 +244,27 @@ func main() { // Retries every unassigned pending booking on a timer, so a booking whose // retry window closed while no rider was free is still picked up later. go assignment.StartPendingSweeper() + // Recalibrates the ETA estimate from the system's own past predictions + // (bookingassignments.etaminutes) against what actually happened. Until + // ROUTE_OPTIMIZER_URL is configured there is nothing to calibrate, the + // calibration stays empty, and every ETA comes from the existing promise + // tables exactly as before. See docs/prediction-plan.md rung 1.1. + go prediction.StartCalibrationSweeper() + // Judges what the engine's past decisions actually achieved, which is what + // makes them retrievable: /internal/agent-decisions/similar filters on + // `outcome IS NOT NULL`, so an unjudged decision is invisible to recall. + // Also prunes past the retention window — this table had none. + // Skill-finding retention rides the outcome sweeper's tick. Assigned here + // rather than called directly, to keep internal/ai/outcomes free of a + // controllers import. + outcomes.PruneFindings = controllers.PruneAIFindings + go outcomes.StartOutcomeSweeper() // 9. Point the stop sequencer at the Route Optimization API. routing.BaseURL = cfg.RouteOptimizerURL + // Where AI_engine's health surface lives, for the console's agent-status + // proxy. Empty disables it and the console keeps using its snapshot. + controllers.AIEngineBaseURL = cfg.AIEngineBaseURL // 10. Record AI_engine agent runs (telemetry.task) and live state // (telemetry.agent). Observability only: a nil NATS connection logs and diff --git a/migrations/migrate.go b/migrations/migrate.go index a77871d..08469d0 100644 --- a/migrations/migrate.go +++ b/migrations/migrate.go @@ -42,6 +42,8 @@ func Migrate(db *gorm.DB) error { &models.CarrierPricing{}, &models.DoormilePricing{}, &models.AgentDecision{}, + &models.AISkillFinding{}, + &models.DemandForecast{}, &models.HubStaffAccount{}, &models.HubConversation{}, &models.HubMessage{}, @@ -89,6 +91,23 @@ func Migrate(db *gorm.DB) error { utils.Error("⚠️ AI registry seed failed", "error", err.Error()) } + // pgvector must exist before the vector column can be declared. This was + // missing: the ALTER TABLE below has always run against a database where + // the extension was installed by hand, and its failure is logged + // non-fatally — so on any database where it was not, the column and its + // index silently never existed and every similarity query 500s. + // + // Creating an extension needs rights a plain application role may not + // have. A failure here is logged and not fatal for the same reason the + // column add is not: the registry and decision log are not on a booking's + // path, and the API must keep serving orders. + if res := db.Exec(`CREATE EXTENSION IF NOT EXISTS vector`); res.Error != nil { + utils.Error("⚠️ pgvector extension unavailable — decision memory and similarity search are disabled", + "error", res.Error) + } else { + utils.Info("✅ pgvector extension ready") + } + if res := db.Exec(`ALTER TABLE agent_decisions ADD COLUMN IF NOT EXISTS context_embedding vector(1536)`); res.Error != nil { utils.Error("❌ Failed to add context_embedding column", "error", res.Error) } else { @@ -169,5 +188,105 @@ func Migrate(db *gorm.DB) error { utils.Info("✅ bookingstageevents (bookingid, occurredat) index ready") } + // Reverse logistics for clients onboarded before the field existed. + // + // They have no delivery category and were all using returns, so they must + // keep them — new information must not withdraw a working capability. Done + // as a one-off backfill rather than a column default: a default makes GORM + // omit an explicit `false` on every future INSERT, which is how Food + // clients were silently created with returns enabled. + // + // Guarded on deliverycategory being empty, so it only ever touches rows + // that predate the field and can never re-enable returns for a client an + // operator has deliberately switched off. + if res := db.Exec(`UPDATE tenants SET reverselogisticsenabled = true + WHERE COALESCE(deliverycategory, '') = ''`); res.Error != nil { + utils.Error("❌ Failed to backfill tenants.reverselogisticsenabled", "error", res.Error) + } else if res.RowsAffected > 0 { + utils.Info("✅ reverse logistics kept on for pre-existing clients", "rows", res.RowsAffected) + } + + // ── ETA calibration (docs/prediction-plan.md rung 1.1) ────────────────── + // + // consignment_booking resolves a consignment to its booking. There is no + // consignments.bookingid column, and the link runs two ways: through + // bookingdestinations for multi-destination customer pickups, and through + // the legacy pickupbookings.consignmentid for console/express bookings and + // anything written before the fan-out existed. That legacy column names + // only the FIRST order of a multi-destination pickup, which is why the + // bookingdestinations path is tried first and the fallback is guarded. + // + // Every query that needs a consignment's booking goes through this view. + // Joining consignments to bookings by hand attaches features to the wrong + // parcel for multi-destination bookings, silently — the defect CLAUDE.md + // §8.5 describes and docs/prediction-plan.md records as hazard H4. + if res := db.Exec(`CREATE OR REPLACE VIEW consignment_booking AS + SELECT c.consignmentid, + COALESCE(bd.bookingid, pb.bookingid) AS bookingid, + bd.bookingdestinationid + FROM consignments c + LEFT JOIN bookingdestinations bd ON bd.consignmentid = c.consignmentid + LEFT JOIN pickupbookings pb ON pb.consignmentid = c.consignmentid + AND bd.bookingid IS NULL`); res.Error != nil { + utils.Error("❌ Failed to create consignment_booking view", "error", res.Error) + } else { + utils.Info("✅ consignment_booking view ready") + } + + // etacalibration holds the grouped p80 of actual-over-routed duration that + // internal/prediction multiplies a routed ETA by. Written only by the + // calibration sweeper, read at boot. Empty is the normal state until + // ROUTE_OPTIMIZER_URL is configured and deliveries accumulate — the + // estimator falls back to the existing promise tables while it is. + if res := db.Exec(`CREATE TABLE IF NOT EXISTS etacalibration ( + calibrationid SERIAL PRIMARY KEY, + scope VARCHAR(24) NOT NULL, + zone VARCHAR(8), + hourbucket INTEGER, + weekday INTEGER, + factor DOUBLE PRECISION NOT NULL, + handling DOUBLE PRECISION NOT NULL DEFAULT 0, + samples INTEGER NOT NULL, + refreshedat TIMESTAMPTZ NOT NULL + )`); res.Error != nil { + utils.Error("❌ Failed to create etacalibration table", "error", res.Error) + } else { + utils.Info("✅ etacalibration table ready") + } + + if res := db.Exec(`CREATE INDEX IF NOT EXISTS idx_etacalibration_lookup + ON etacalibration (scope, zone, hourbucket, weekday)`); res.Error != nil { + utils.Error("❌ Failed to create etacalibration lookup index", "error", res.Error) + } else { + utils.Info("✅ etacalibration lookup index ready") + } + + // The calibration scan joins deliveries to their assignment. Without this + // the refresh sequential-scans consignmenthistory on every run. + // + // CONCURRENTLY is not optional here. A plain CREATE INDEX takes a SHARE + // lock, which blocks INSERTs for as long as the build takes — and + // consignmenthistory gets a row on EVERY parcel status change. On a table + // of any size that means riders cannot complete deliveries while the + // migration runs: a revenue-path outage caused by an index for a + // background job that is switched off by default. + // + // The tradeoffs CONCURRENTLY brings, and why they are acceptable: + // - It cannot run inside a transaction. db.Exec is autocommit, so this + // is fine, but it must never be moved inside a tx block. + // - It can fail and leave an INVALID index behind, which is then not + // used by the planner and must be dropped by hand. That degrades the + // calibration scan to a sequential one; it does not affect any + // booking. The log line below names the recovery. + if res := db.Exec(`CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_consignmenthistory_status_consignment + ON consignmenthistory (eventstatus, consignmentid)`); res.Error != nil { + utils.Error("⚠️ consignmenthistory index not created — the ETA calibration scan will be slower. "+ + "If this left an INVALID index, drop it before retrying: "+ + "DROP INDEX IF EXISTS idx_consignmenthistory_status_consignment", + "error", res.Error) + } else { + utils.Info("✅ consignmenthistory (eventstatus, consignmentid) index ready") + } + return nil } diff --git a/models/agentdecision.go b/models/agentdecision.go index e58728f..05d11ff 100644 --- a/models/agentdecision.go +++ b/models/agentdecision.go @@ -3,9 +3,22 @@ package models import "time" type AgentDecision struct { - ID uint64 `gorm:"primaryKey"` - DecisionType string `gorm:"size:50;index"` - BookingID *uint64 `gorm:"index"` + ID uint64 `gorm:"primaryKey"` + DecisionType string `gorm:"size:50;index"` + BookingID *uint64 `gorm:"index"` + // TenantID scopes retrieval. Without it, similarity search would return one + // client's operational history as precedent for a decision about another — + // and because the engine feeds that precedent to the model, one tenant's + // past would shape another tenant's dispatch. + // + // Nullable because the engine does not always know the tenant (a B2C + // booking carries a nil tenantid by design). A NULL row is recallable only + // by a NULL-tenant query, never by a tenant's own — the safer default, + // given console logins are already unscoped when tenantid is NULL. + // + // Added before the table filled on purpose: adding it afterwards is a + // backfill with no way to attribute the rows already there. + TenantID *uint64 `gorm:"column:tenantid;index"` Context string `gorm:"type:jsonb"` Decision string `gorm:"type:jsonb"` Reasoning string `gorm:"type:text"` diff --git a/models/ai_findings.go b/models/ai_findings.go new file mode 100644 index 0000000..f5dad7b --- /dev/null +++ b/models/ai_findings.go @@ -0,0 +1,92 @@ +package models + +import "time" + +// AISkillFinding is one thing a console ops skill noticed. +// +// ─── Why this table exists ───────────────────────────────────────────────── +// +// The eight rule skills (SlaGuardian, DoorstepStall, FleetBalancer, +// HighValueCod, RiderBatterySafety, HubCongestion, LateDispatch, CashExposure) +// run in the operator's browser, on a 60-second React Query interval, and their +// findings were thrown away. Nothing persisted them, so three questions had no +// answer at all: +// +// Has this booking been flagged before, and how many times? +// Which skills fire most, and which are ignored every time they do? +// Did the proposal an operator carried out actually clear the finding? +// +// The third is the one that matters. The console already re-runs its scan after +// executing a proposal to see whether the finding disappears — that evidence +// existed for one render and then vanished. Persisting it turns "the skills seem +// useful" into something measurable, and it is the raw material for any later +// per-rider or per-zone memory. +// +// ─── What this is NOT ────────────────────────────────────────────────────── +// +// Not an exception. `consignmentexceptions` records a real operational problem +// with a parcel (Lost, Damaged, Misrouted) raised by a person. A finding is a +// rule noticing a pattern, most of which resolve themselves without anyone +// doing anything. Writing findings into that table would flood a queue people +// are supposed to work. +// +// Not a decision either: `agent_decisions` is the engine's model-made choices, +// with embeddings for retrieval. A finding is deterministic and carries no +// vector. +type AISkillFinding struct { + Findingid int `json:"findingid" gorm:"primaryKey;column:findingid;autoIncrement"` + + // Skillid matches aiskills.skillid, so a finding can be grouped by the rule + // that raised it and read alongside the thresholds in force at the time. + Skillid string `json:"skillid" gorm:"column:skillid;size:64;not null;index:idx_aiskillfindings_skill_time,priority:1"` + + // Fingerprint is what makes a finding the SAME finding across polls. The + // skills re-evaluate every 60 seconds and will re-raise an unchanged + // problem every time; without this the table would grow by the number of + // open findings per minute per operator with the page open. + // + // Computed by the console from the skill id and the scope's booking ids — + // deliberately not including severity or counts, which drift while the + // underlying problem stays the same. + Fingerprint string `json:"fingerprint" gorm:"column:fingerprint;size:120;not null;uniqueIndex:uq_aiskillfindings_fingerprint"` + + Severity string `json:"severity" gorm:"column:severity;size:20"` // critical, warning, info + Title string `json:"title" gorm:"column:title"` + + // Proposaltool is the verb the finding proposes, e.g. notifyRider, + // assignMiler, enforce_cash_handoff. Matches aitools.toolname where one + // exists; review-only verbs are recorded too, which is how "this skill + // keeps proposing something nobody can act on" becomes visible. + Proposaltool string `json:"proposaltool" gorm:"column:proposaltool;size:64;index"` + + // Bookingcount and Scope: the count is for grouping, the scope is the + // booking ids as a JSON array so a specific parcel's history can be found. + // Not a join table — a finding's scope is read whole or not at all, and a + // row per booking per finding would multiply this table by average scope + // size for no query anyone needs. + Bookingcount int `json:"bookingcount" gorm:"column:bookingcount;default:0"` + Scope string `json:"scope" gorm:"column:scope;type:jsonb"` + + Tenantid *int `json:"tenantid" gorm:"column:tenantid;index"` + + // Firstseenat vs Lastseenat: an upsert on fingerprint bumps the last-seen + // and the count, so how LONG a finding has been open is answerable. That is + // the actual signal of an ignored finding — not that it exists, but that it + // has existed for six hours. + Firstseenat time.Time `json:"firstseenat" gorm:"column:firstseenat;not null;index:idx_aiskillfindings_skill_time,priority:2"` + Lastseenat time.Time `json:"lastseenat" gorm:"column:lastseenat;not null"` + Seencount int `json:"seencount" gorm:"column:seencount;default:1"` + + // Actedat / Actedby / Actionresult record an operator carrying out the + // proposal. Null means nobody did — which is data, not a gap. + Actedat *time.Time `json:"actedat" gorm:"column:actedat"` + Actedby *int `json:"actedby" gorm:"column:actedby"` + Actionresult string `json:"actionresult" gorm:"column:actionresult;size:20"` // ok, partial, failed + + // Clearedat is set when a later scan no longer raises this fingerprint. + // Paired with Actedat it answers the question the whole table is for: did + // acting clear it, or did it clear on its own? + Clearedat *time.Time `json:"clearedat" gorm:"column:clearedat"` +} + +func (AISkillFinding) TableName() string { return "aiskillfindings" } diff --git a/models/demand_forecast.go b/models/demand_forecast.go new file mode 100644 index 0000000..4b6b30d --- /dev/null +++ b/models/demand_forecast.go @@ -0,0 +1,53 @@ +package models + +import "time" + +// DemandForecast is one zone-day the engine expects. +// +// ─── Why this is stored rather than computed on read ─────────────────────── +// +// The forecast is produced in Python (AI_engine/prediction), because the model +// is: Prophet where it beats a seasonal baseline, the baseline otherwise. The +// console and any staffing decision need it in Go. So the engine writes here +// and the backend serves it — the same split the agent registry and the +// decision log already use, and the reason the /internal/* surface exists. +// +// It is also the honest shape: a forecast is a thing produced at a moment by a +// model, not a function of the current table. Recomputing it on every read +// would make yesterday's number unrecoverable, which is exactly what you want +// when asking "was the forecast any good". +type DemandForecast struct { + Forecastid int `json:"forecastid" gorm:"primaryKey;column:forecastid;autoIncrement"` + + // Zone is the first three digits of a pickup pincode — the same grain the + // hub console scopes on (pickuppincode LIKE '641%') and the same grain + // internal/prediction calibrates ETA at. Keeping one definition of "zone" + // across both is deliberate. + Zone string `json:"zone" gorm:"column:zone;size:8;not null;uniqueIndex:uq_demandforecast_zone_day,priority:1"` + + // Forday is the day being predicted, not the day it was predicted on. + Forday time.Time `json:"forday" gorm:"column:forday;not null;uniqueIndex:uq_demandforecast_zone_day,priority:2;index"` + + Expectedbookings int `json:"expectedbookings" gorm:"column:expectedbookings;not null"` + + // Model and Reason record WHICH model produced this and why it was chosen — + // "prophet beat the weekly baseline over 12 folds (18% lower MAE)", or + // "prophet is not installed in this image". Stored because the choice is + // made per zone from that zone's own history, so without it nobody can tell + // whether a bad forecast came from a bad model or from a thin series. + Model string `json:"model" gorm:"column:model;size:32;not null"` + Reason string `json:"reason" gorm:"column:reason"` + + // Observations is how many days of history the forecast was fitted on, and + // Baselinemae/Modelmae are the backtest scores. A forecast with 31 + // observations and a model barely beating the baseline deserves less trust + // than one with 400, and this is what lets a reader see that rather than + // taking the number at face value. + Observations int `json:"observations" gorm:"column:observations;default:0"` + Baselinemae *float64 `json:"baselinemae" gorm:"column:baselinemae"` + Modelmae *float64 `json:"modelmae" gorm:"column:modelmae"` + + Generatedat time.Time `json:"generatedat" gorm:"column:generatedat;not null"` +} + +func (DemandForecast) TableName() string { return "demandforecast" } diff --git a/models/external.go b/models/external.go index 6495a1a..ca81252 100644 --- a/models/external.go +++ b/models/external.go @@ -14,11 +14,59 @@ type Tenant struct { // parcel; not for a food order, where it just slows every drop down. // Defaults off: turning it on platform-wide would block deliveries for // clients whose customer app has no way to show the code yet. - Requiredeliveryotp bool `json:"requiredeliveryotp" gorm:"column:requiredeliveryotp;default:false"` + Requiredeliveryotp bool `json:"requiredeliveryotp" gorm:"column:requiredeliveryotp;default:false"` + // Deliverycategory is WHAT this client ships, from the same vocabulary + // pricing uses (constants.DeliveryCategories) — a tenant whose category is + // not a pricing category cannot be priced, so the two are one list. + // + // It decides whether reverse logistics applies: a returned meal is waste, + // not inventory, so Food clients get no RTO path. See + // constants.ReverseLogisticsAllowed for the reasoning. + // + // Empty on every tenant onboarded before this field existed, and empty + // must keep behaving as it always did — which is why + // ReverseLogisticsAllowed treats an unknown category as returnable rather + // than defaulting the new flag to off. + Deliverycategory string `json:"deliverycategory" gorm:"column:deliverycategory;size:32"` + // Reverselogisticsenabled is the OPERATIONAL consequence, stored rather + // than derived on every read. + // + // Two reasons it is its own column. First, an operator may need to turn + // returns off for a non-Food client (a clearance line that is final sale) + // or on for a Food client (a caterer who takes back equipment), and a + // derived value cannot be overridden. Second, deriving it would mean that + // editing a client's category silently changes whether live parcels can be + // returned — a stored flag makes that an explicit second decision. + // A POINTER, deliberately, and this is load-bearing. + // + // GORM omits a zero-value field from an INSERT when the tag declares a + // default — so a plain `bool` set to false was silently dropped and the + // column default (true) applied, creating Food clients with reverse + // logistics ENABLED: the exact case this feature exists to prevent, + // failing silently. A test caught it. + // + // Dropping the default instead is worse: ADD COLUMN ... NOT NULL with no + // default fails outright on a populated tenants table. + // + // A pointer gives both. nil means "not stated" and the column default + // (true) applies — which is what every row predating this field wants. A + // non-nil false is written as false, because GORM never omits a non-nil + // pointer. Read it through ReturnsEnabled(), never directly. + Reverselogisticsenabled *bool `json:"reverselogisticsenabled" gorm:"column:reverselogisticsenabled;default:true"` Createdat time.Time `json:"createdat" gorm:"column:createdat;default:CURRENT_TIMESTAMP"` Updatedat time.Time `json:"updatedat" gorm:"column:updatedat;default:CURRENT_TIMESTAMP"` } +// ReturnsEnabled is the one way to read Reverselogisticsenabled. +// +// nil means the client predates the field, and those clients were all using +// returns — so nil is TRUE. Reading the pointer directly invites a nil deref +// or, worse, treating "not stated" as "disabled" and silently withdrawing a +// capability a client is already using. +func (t Tenant) ReturnsEnabled() bool { + return t.Reverselogisticsenabled == nil || *t.Reverselogisticsenabled +} + func (Tenant) TableName() string { return "tenants" } diff --git a/routes/routes.go b/routes/routes.go index 07e9252..ae9e203 100644 --- a/routes/routes.go +++ b/routes/routes.go @@ -415,6 +415,13 @@ func RegisterRoutes(app *fiber.App, cfg *config.Config) { adminAuth.Put("/bookings/:id/status", controllers.AdminUpdateBookingStatus) adminAuth.Post("/bookings/:id/cancel", controllers.AdminCancelBooking) adminAuth.Post("/bookings/bulk-cancel", controllers.AdminBulkCancelBookings) + // Batch assign, admin side. The same solver /hub/bookings/batch-assign + // uses (controllers/batchAssignService.go), with admin auth and scoped to + // the login's own tenant. This is what backs the console ops layer's + // `assignMiler` proposal, which had no executor because the hub route + // 403s for every admin token and /bookings/:id/assign-miler needs a rider + // the finding does not pick. + adminAuth.Post("/bookings/batch-assign", middlewares.DoormileStaffOnly, controllers.AdminBatchAssign) // Consignments adminAuth.Get("/consignments", controllers.GetAdminConsignments) @@ -486,6 +493,23 @@ func RegisterRoutes(app *fiber.App, cfg *config.Config) { aiRegistry.Patch("/skills/:id", middlewares.RoleCheckMiddleware(1), controllers.PatchAISkill) aiRegistry.Get("/tools", controllers.GetAITools) aiRegistry.Get("/audit", controllers.GetAIRegistryAudit) + // What the console's rule skills have been noticing. The findings used to + // exist for one render in the operator's browser and then vanish, so + // nothing could say whether a skill was useful or whether acting on a + // finding cleared it. Writes are open to the same roles as reads here: + // the console reports what it evaluated, it is not an operator action. + aiRegistry.Get("/findings", controllers.GetAIFindings) + aiRegistry.Get("/findings/stats", controllers.GetAIFindingStats) + aiRegistry.Post("/findings", controllers.UpsertAIFindings) + aiRegistry.Post("/findings/:fingerprint/acted", controllers.RecordAIFindingActed) + // Tomorrow's expected pickups per zone, with the staffing gap against + // riders actually on duty. A bare forecast number is not actionable — the + // gap is (docs/prediction-plan.md §2.4). + aiRegistry.Get("/forecast/demand", controllers.GetDemandForecast) + // The engine's live agent state, proxied. The console's Agents page cannot + // reach :8700 itself (ClusterIP, and it is a browser). A 503 here means + // "use the snapshot", not "broken" — see the controller. + aiRegistry.Get("/engine/agents", controllers.GetAIEngineAgents) // What the agents did (Phase 4): runs from AI_engine telemetry, decisions // from agent_decisions, live heartbeat from Redis. Read-only. aiRegistry.Get("/insights", controllers.GetAIInsights) @@ -591,10 +615,19 @@ func RegisterRoutes(app *fiber.App, cfg *config.Config) { internal.Post("/notify", controllers.InternalNotify) internal.Post("/bookings/:id/reassign", controllers.InternalReassign) internal.Post("/agent-decisions", controllers.CreateAgentDecision) - internal.Get("/agent-decisions/similar", controllers.FindSimilarDecisions) + // POST, not GET: the handler needs a 1536-float embedding in the body, and + // a GET with a body is dropped by nginx and most HTTP clients — and this + // API is served THROUGH host nginx today (conf/nginx-doormile.conf), so it + // would not merely be risky, it would not work. Nothing called it while it + // was a GET, so changing the method breaks no caller. + internal.Post("/agent-decisions/similar", controllers.FindSimilarDecisions) internal.Patch("/agent-decisions/:id/outcome", controllers.UpdateDecisionOutcome) // The agent registry, for AI_engine to poll (ETag / If-None-Match → 304). internal.Get("/ai/registry", controllers.GetInternalAIRegistry) + // The demand forecast, written by AI_engine's forecast job. The model lives + // in Python (prophet where it beats a seasonal baseline, the baseline + // otherwise); the backend stores and serves it. + internal.Post("/demand-forecast", controllers.UpsertDemandForecast) // Express-batch dispatch: the ExpressDispatchAgent reads a tenant's riders // and the batch's bookings, then writes back the assignments it decided. diff --git a/routes/routes_ai_registry_test.go b/routes/routes_ai_registry_test.go index 0c0eed7..dfff833 100644 --- a/routes/routes_ai_registry_test.go +++ b/routes/routes_ai_registry_test.go @@ -36,6 +36,14 @@ var aiReads = []string{ "/api/v1/admin/ai/insights?days=30", "/api/v1/admin/ai/decisions", "/api/v1/admin/ai/status", + // Added with the agent-platform work. Listed here so the existing + // invariants cover them: every /admin/ai read must require a login and + // must refuse miler, hub-staff and customer roles. A new route that skips + // this table is a new route nobody proved is gated. + "/api/v1/admin/ai/findings", + "/api/v1/admin/ai/findings/stats", + "/api/v1/admin/ai/forecast/demand", + "/api/v1/admin/ai/engine/agents", } var aiWrites = []struct{ method, path, body string }{ @@ -45,6 +53,46 @@ var aiWrites = []struct{ method, path, body string }{ {http.MethodPost, "/api/v1/admin/ai/playground/run", `{"agentid":"EXCEPTION_AGENT","prompt":"hi"}`}, } +// Finding reports are NOT registry mutations, and deliberately not role-1-only. +// +// The aiWrites table above encodes "changing the registry is an operator +// decision, so role 1 only". Reporting what the rule skills noticed is a +// different thing: it is telemetry from the Exceptions page, which managers (3) +// and executives (4) open as part of their job. Gating it to role 1 would mean +// a manager's session silently reported nothing, and the "how long has this +// finding been open" measurement would depend on who happened to be logged in. +// +// What it must still refuse is everyone outside the console: miler, hub staff +// and customer tokens. +var aiFindingWrites = []struct{ method, path, body string }{ + {http.MethodPost, "/api/v1/admin/ai/findings", `{"findings":[],"cleared":[]}`}, + {http.MethodPost, "/api/v1/admin/ai/findings/abc/acted", `{"result":"ok"}`}, +} + +func TestFindingReportsAreOpenToConsoleRolesButNotOutsiders(t *testing.T) { + app := newApp() + for _, w := range aiFindingWrites { + // No token at all is refused. + if code, _ := do(t, app, w.method, w.path, "", w.body); code != http.StatusUnauthorized { + t.Errorf("%s %s with no token = %d, want 401", w.method, w.path, code) + } + // Outside the console: refused. + for _, role := range []int{5, 6, 9} { // miler, hub staff, customer + if code, _ := do(t, app, w.method, w.path, token(t, 1, role), w.body); code != http.StatusForbidden && code != http.StatusUnauthorized { + t.Errorf("%s %s as role %d = %d, want 401/403", w.method, w.path, role, code) + } + } + // Console roles: NOT forbidden. The handler may still fail without a + // database in this app; what matters here is that authorisation let it + // through rather than stopping it. + for _, role := range []int{1, 3, 4} { + if code, _ := do(t, app, w.method, w.path, token(t, 1, role), w.body); code == http.StatusForbidden { + t.Errorf("%s %s as role %d = 403; the Exceptions page must be able to report findings", w.method, w.path, role) + } + } + } +} + func TestAIRegistryRequiresALogin(t *testing.T) { app := newApp() for _, p := range aiReads { diff --git a/routes/routes_client_onboarding_pg_test.go b/routes/routes_client_onboarding_pg_test.go index 58cae6e..e269a2a 100644 --- a/routes/routes_client_onboarding_pg_test.go +++ b/routes/routes_client_onboarding_pg_test.go @@ -54,7 +54,7 @@ func onboardingDB(t *testing.T) *gorm.DB { func onboardBody(extra string) string { return `{"companyname":"Peelamedu Provisions","contactname":"Priya Raman","email":"ops@peelamedu.test", - "phone":"9876543210","password":"Strong-pass-1","applocationid":1` + extra + `}` + "phone":"9876543210","password":"Strong-pass-1","applocationid":1,"deliverycategory":"Clothing"` + extra + `}` } const goodAddress = `,"address":"14 DB Road, RS Puram, Coimbatore","city":"Coimbatore","state":"Tamil Nadu", diff --git a/scratch/prediction_readiness.sql b/scratch/prediction_readiness.sql new file mode 100644 index 0000000..52746c6 --- /dev/null +++ b/scratch/prediction_readiness.sql @@ -0,0 +1,137 @@ +-- Doormile — prediction readiness queries (Phase 0 + rung 1.0) +-- docs/prediction-plan.md §2 and §3 +-- +-- ALL READ-ONLY. No DDL, no writes. Safe to run against production. +-- Run top to bottom; each block prints one answer the plan needs. +-- +-- Two of these depend on the consignment_booking view that migrate.go now +-- creates (hazard H4 — consignments has no bookingid column). If the view does +-- not exist yet, the blocks marked [needs view] will error; everything else +-- still answers. + +\echo '=== Q1. Is routing even running? (gates rung 1.1) ===' +-- with_routed_eta = 0 means ROUTE_OPTIMIZER_URL is unset in the cluster and +-- there is nothing to calibrate yet. See Phase 7 Track A1. +SELECT count(*) AS assignments, + count(*) FILTER (WHERE etaminutes > 0) AS with_routed_eta, + count(*) FILTER (WHERE step > 0) AS sequenced, + min(sequencedat) AS first_sequenced, + max(sequencedat) AS last_sequenced +FROM bookingassignments; + +\echo '=== Q2. How much delivery history exists? (gates everything) ===' +SELECT count(*) AS delivered_events, + min(createdat)::date AS first_day, + max(createdat)::date AS last_day, + count(DISTINCT createdat::date) AS distinct_days +FROM consignmenthistory +WHERE eventstatus = 'Delivered'; + +\echo '=== Q3. H2 — delivered consignments with no Delivered event (silent label loss) ===' +-- Non-zero means the training label is missing for those rows. Fix the write +-- path before fitting anything. +SELECT count(*) AS delivered_without_event +FROM consignments c +LEFT JOIN consignmenthistory h + ON h.consignmentid = c.consignmentid AND h.eventstatus = 'Delivered' +WHERE c.status = 'Delivered' AND h.consignmentid IS NULL; + +\echo '=== Q4. H4 — can every consignment be resolved to a booking? [needs view] ===' +SELECT count(*) AS consignments, + count(*) FILTER (WHERE bookingid IS NULL) AS unresolvable +FROM consignment_booking; + +\echo '=== Q5. Per-pincode series length (gates demand grain) ===' +-- Demand SARIMA/Prophet needs ~90 days PER SERIES, not in total. +-- If days_with_data is small everywhere, aggregate to city level. +SELECT c.deliverypincode, + count(DISTINCT h.createdat::date) AS days_with_data, + count(*) AS deliveries +FROM consignmenthistory h +JOIN consignments c ON c.consignmentid = h.consignmentid +WHERE h.eventstatus = 'Delivered' +GROUP BY 1 +ORDER BY 3 DESC +LIMIT 30; + +\echo '=== Q6. Rung 1.0 — how wrong are the promise constants? [needs view] ===' +-- Actual duration uses consignmenthistory.createdat - consignments.createdat. +-- Both come from CURRENT_TIMESTAMP defaults, so they are consistent with each +-- other and this comparison is unaffected by H1. +-- +-- Deliberately NOT comparing against estimateddeliveryat: that column is +-- written with time.Now() at adminController.go:2738 but CURRENT_TIMESTAMP +-- elsewhere, so its tagging is not uniform. Promise hours are taken from the +-- service type instead, which is unambiguous. +SELECT so.servicetype, + count(*) AS n, + round(avg(EXTRACT(EPOCH FROM (h.createdat - c.createdat))/3600)::numeric, 2) AS actual_hours_avg, + round(percentile_cont(0.5) WITHIN GROUP ( + ORDER BY EXTRACT(EPOCH FROM (h.createdat - c.createdat))/3600)::numeric, 2) AS actual_hours_p50, + round(percentile_cont(0.8) WITHIN GROUP ( + ORDER BY EXTRACT(EPOCH FROM (h.createdat - c.createdat))/3600)::numeric, 2) AS actual_hours_p80, + round(percentile_cont(0.95) WITHIN GROUP ( + ORDER BY EXTRACT(EPOCH FROM (h.createdat - c.createdat))/3600)::numeric, 2) AS actual_hours_p95 +FROM consignments c +JOIN consignmenthistory h ON h.consignmentid = c.consignmentid AND h.eventstatus = 'Delivered' +JOIN consignment_booking cb ON cb.consignmentid = c.consignmentid +LEFT JOIN bookingserviceoptions so ON so.bookingid = cb.bookingid +GROUP BY 1 +ORDER BY 2 DESC; + +\echo '=== Q7. Rung 1.0 — SLA breach rate against the promise actually stored ===' +SELECT count(*) AS delivered, + count(*) FILTER (WHERE c.sladueat IS NOT NULL) AS with_sla, + count(*) FILTER (WHERE h.createdat > c.sladueat) AS breached +FROM consignments c +JOIN consignmenthistory h ON h.consignmentid = c.consignmentid AND h.eventstatus = 'Delivered'; + +\echo '=== Q8. H3 — do delivery durations look fabricated? ===' +-- Real durations spread. Heavy clustering on round values, or a near-constant +-- offset from createdat, means the timestamps are generated rather than +-- observed. A model fitted on these predicts confidently and wrongly. +SELECT round((EXTRACT(EPOCH FROM (h.createdat - c.createdat))/3600)::numeric, 0) AS duration_hours, + count(*) AS n +FROM consignments c +JOIN consignmenthistory h ON h.consignmentid = c.consignmentid AND h.eventstatus = 'Delivered' +GROUP BY 1 +ORDER BY 2 DESC +LIMIT 20; + +\echo '=== Q9. Demand series at city grain (the safe starting grain) ===' +SELECT left(pickuppincode, 3) AS zone, + count(DISTINCT createdat::date) AS days_with_data, + count(*) AS bookings, + min(createdat)::date AS first_day, + max(createdat)::date AS last_day +FROM pickupbookings +WHERE status <> 'Cancelled' +GROUP BY 1 +ORDER BY 3 DESC; + +\echo '=== Q10. Would the calibration have any cells to fill? [needs view] ===' +-- Mirrors the GROUP BY that internal/prediction/calibration.go uses. Cells +-- below the sample floor are discarded, so this says whether rung 1.1 can +-- calibrate at all or must stay on its fallback. +-- Mirrors internal/prediction/calibration.go exactly, including the DISTINCT ON +-- that picks ONE assignment per booking. A booking passes through several +-- bookingassignments rows (Assigned, Rejected, Reassigned), so a plain join +-- multiplies each delivery by its assignment history and overstates samples. +WITH assigned AS ( + SELECT DISTINCT ON (ba.bookingid) ba.bookingid, ba.etaminutes + FROM bookingassignments ba + WHERE ba.etaminutes > 0 + ORDER BY ba.bookingid, ba.sequencedat DESC NULLS LAST, ba.bookingassignmentid DESC +) +SELECT left(c.deliverypincode, 3) AS zone, + (EXTRACT(HOUR FROM c.createdat)::int / 3) AS hour_bucket, + EXTRACT(ISODOW FROM c.createdat)::int AS weekday, + count(*) AS samples +FROM consignments c +JOIN consignmenthistory h ON h.consignmentid = c.consignmentid AND h.eventstatus = 'Delivered' +JOIN consignment_booking cb ON cb.consignmentid = c.consignmentid +JOIN assigned a ON a.bookingid = cb.bookingid +GROUP BY 1, 2, 3 +HAVING count(*) >= 20 +ORDER BY 4 DESC +LIMIT 25;