diff --git a/docs/agent-platform-phase7-plan.md b/docs/agent-platform-phase7-plan.md new file mode 100644 index 0000000..64167e8 --- /dev/null +++ b/docs/agent-platform-phase7-plan.md @@ -0,0 +1,777 @@ +# Doormile Agent Platform — Phase 7: Decision Memory, Deployment, Executors + +Status: **mostly implemented 2026-10-08 (uncommitted, not deployed, no migration run)** +Written 2026-10-08 · Scope: `krow_talent_app`, `doormile_backend`, `AI_engine`, `kubernetes` + +--- + +## Status (2026-10-08) + +Verified: backend `go build`/`go vet` clean, 20 packages pass · engine 148 tests +(3 pre-existing import errors) · console 1397 pass (13 pre-existing +`agentStudio.test.jsx` failures) · `kubectl kustomize manifests/doormile` renders +clean. **Nothing committed. No migration has run. No SQL in this work has ever +executed against a database.** + +| Item | State | +|---|---| +| A1 config/secrets manifest, `envFrom`, probes, resources | **built** — placeholders unreplaced | +| A2 secrets out of git | **not done** — documented in `manifests/doormile/SECRETS.md`, including the kustomize-clobbers-hand-created-secrets trap. Five more placeholder keys were added. | +| A3 `anthropic>=0.49.0` · A4 `claude-opus-5-5` | **done** | +| B0.1 `CREATE EXTENSION vector` · B0.2 `/similar` → POST | **done** | +| B1 provider (OpenAI `text-embedding-3-small`, 1536) | **done** — my call, flagged | +| B2 `core/embeddings.py` + decision wiring | **done** | +| B3 outcome sweeper + rules + 14 tests | **done** | +| B4 `core/memory.py` + both agents | **done** | +| B5 retention/prune | **done** — ivfflat `lists` re-tune still needs a row count | +| B6 `tenantid` scoping | **done** | +| B2.5 historical backfill | **done** — `internal/ai/outcomes/backfill.go` | +| B7 persist console findings | **done** — `aiskillfindings`, 4 routes, `findingReport.js`, 15 tests | +| C1 health surface + `ai-engine.yaml` + Dockerfile/compose port | **done** — image not built or pushed | +| C2 kustomization + `deploy-doormile.sh` | **done** | +| C3 probes/resources/PDB | **done** — StatefulSet→Deployment not done | +| C4 ingress | **built** — apply order matters, see the file header | +| C5 NATS port note | **done** — `docs/ARCHITECTURE.md` | +| C6 engine replica safety | **see correction 1 — was already safe for a different reason** | +| D1 admin batch-assign + console executor | **done** | +| D2 `alert_low_battery_rider` | **done** | +| D3 five review-only tools | **1 of 5 done** — `trigger_auto_dispatch` turned out to BE batch-assign under another name. The other four need endpoints that do not exist. | +| Agents-page snapshot → live endpoint | **backend done** — engine serves `GET /agents/status`; the console still reads the snapshot, so the swap is one fetch away | + +## Two things this plan got wrong + +**Correction 1 — C6. The engine's stall dedup was never per-pod.** +This plan claimed `ExceptionAgent` kept its dedup in an in-process TTL set and +that moving it to Redis was the prerequisite for scaling out. Wrong: +`_claim_stall` / `_claim_stall_handled` (`agents/exception_agent.py:336`) have +been Redis `SET NX` with a TTL all along — cross-pod, self-expiring, durable +across restarts. The comment there saying "replaces the old unbounded in-memory +set" describes what it REPLACED; I read it as current state. + +The real blocker is different and still real: `core/message_bus.py:284` and +`:312` bind **durable push consumers with fixed names**, and a durable push +consumer admits one active subscriber unless created with a deliver group. A +second replica does not duplicate work — it fails to bind. Scaling out means +giving those subscriptions a deliver group (or moving to pull consumers). +`replicas: 1` stands, for the corrected reason. + +**Correction 2 — the registry was already honest about the simulated agents.** +This plan said the three fictional-data agents were "presented as live" and +should be marked. They already were: `seed.go` has `StatusSimulation` for +`HUB_AGENT`, `FLEET_AGENT` and `ROUTE_OPTIMIZER`, with purposes reading "an +in-memory fleet of 19 fake vehicles" and "8 hard-coded fictional hubs". The +console snapshot (`src/lib/agentNetwork.js`) was also honest about activity +("Zero tasks received", `publishes: 'Nothing on the bus.'`, `api: 'No.'`) but +silent on the data being invented — so the three entries now say so, and the two +surfaces agree. + +I overstated that surface as "telling anyone something untrue". It was +incomplete, not false. + +--- + +## Track A — Unblock (do this first; nothing else matters until it lands) + +### A1. `INTERNAL_API_KEY` is missing from the cluster — **everything engine↔backend is 401** + +`middlewares/internal_auth.go:13` reads `INTERNAL_API_KEY` and **fails closed when +unset**: + +```go +expected := os.Getenv("INTERNAL_API_KEY") +if expected == "" || c.Get("X-Internal-Key") != expected { +``` + +`kubernetes/manifests/doormile/miletruth.yaml` never sets it. The engine's +`docker-compose.yml` *does* pass it. So the engine sends a key the backend +rejects, and in-cluster **every** `/api/v1/internal/*` call 401s: + +- `GET /internal/ai/registry` — the registry poll (this is why "engine reads + registry in prod" was never confirmable) +- `POST /internal/agent-decisions` — the decision log, so Insights sees nothing +- `GET /internal/express/riders`, `/express/bookings`, `POST /express/assign` +- `POST /internal/bookings/:id/reassign`, `POST /internal/notify` + +**Step 1 — find out whether git matches reality.** The `kubernetes` repo's log is +full of `Restore…` / `Recreate…` commits, so the manifest may already be fiction: + +```bash +kubectl -n doormile get statefulset doormile -o jsonpath='{range .spec.template.spec.containers[0].env[*]}{.name}{"\n"}{end}' | sort +``` + +`config/config.go` reads 29 env vars. If that list is much shorter, the live +cluster has hand-applied drift and **the next `kubectl apply -f` wipes it.** + +**Step 2 — close the gap in git, not by hand.** Add to `doormile-secrets` +(`stringData`) and reference from the StatefulSet. Absent from the manifest today +and read by `config.go`: + +`INTERNAL_API_KEY`, `JWT_SECRET_KEY`, `APP_PORT`, `ENV`, `TRUSTED_PROXIES`, +`AI_LAYER_BASE_URL`, `ROUTE_OPTIMIZER_URL`, `GEOCODER_URL`, `GEOCODER_EMAIL`, +`SMTP_HOST`, `SMTP_PORT`, `SMTP_USER`, `SMTP_PASSWORD`, `SMTP_FROM`, +`CLIENT_ONBOARDING_OWNERS`, `PLAYGROUND_LLM_API_KEY`, `PLAYGROUND_LLM_BASE_URL`, +`PLAYGROUND_LLM_MODEL`, `REDIS_USER`. + +Secrets vs ConfigMap: `INTERNAL_API_KEY`, `JWT_SECRET_KEY`, `SMTP_PASSWORD`, +`PLAYGROUND_LLM_API_KEY` are secrets. The rest belong in a `doormile-config` +ConfigMap so a URL change is not a secret edit. + +**Files** +| File | Change | +|---|---| +| `kubernetes/manifests/doormile/miletruth.yaml` | modify — add the 19 env vars to the StatefulSet + `doormile-secrets` | +| `kubernetes/manifests/doormile/doormile-config.yaml` | **new** — ConfigMap for the non-secret vars | +| `AI_engine/.env` (server-local, untracked content) | modify — same `INTERNAL_API_KEY` value | + +Read-only, no change: `doormile_backend/middlewares/internal_auth.go:13`, +`doormile_backend/config/config.go:60-102` (the contract being satisfied). + +> `INTERNAL_API_KEY` must be **byte-identical** in the backend Secret and the +> engine's env. Generate once (`openssl rand -hex 32`), set both. + +**Acceptance:** from inside the cluster, +`curl -H "X-Internal-Key: $KEY" http://doormile-service.doormile:8081/api/v1/internal/ai/registry` +returns 200 with a body and an `ETag`; a second call with `If-None-Match` returns +304. Backend logs show `ai playground: enabled`. `GET /admin/ai/status` reports +the engine as following the registry (`aiRegistryController.go:208` counts the +poll). + +### A2. Plaintext secrets are committed + +`miletruth.yaml:19-21` has `DB_PASSWORD`, `REDIS_PASSWORD`, `NATS_PASSWORD` as +literal in git (the same password reused for all three). Same class as the committed Firebase keys from the +CX audit. Pick one and apply it before A1 adds *more* secrets to that file: +Sealed Secrets, SOPS, or an out-of-git `kubectl create secret` with the manifest +holding only a reference. + +Do **not** delete or untrack anything here without per-file approval — see the +standing rule. Rotating that shared password is a separate decision; this item is only +about stopping new secrets entering git. + +**Files** +| File | Change | +|---|---| +| `kubernetes/manifests/doormile/miletruth.yaml` | modify — `stringData` block becomes a reference | +| `kubernetes/manifests/doormile/sealed-secrets.yaml` | **new** — if Sealed Secrets is the chosen route | +| `kubernetes/docs/DEPLOY.md` | modify — document how secrets are supplied now | + +Same pattern already in git at `kubernetes/manifests/core/core-secrets.yaml` and +`manifests/nearle/nearle-secrets.yaml` — whatever is chosen should cover those too, +but that is outside this phase. + +### A3. `anthropic>=0.21.0` floor is wrong + +`AI_engine/requirements.txt` pins `anthropic>=0.21.0`, but `core/llm.py:117` +sends `thinking: {"type": "adaptive"}` and `output_config.effort` — parameters +that old SDK does not know. There is no lockfile, so a fresh `pip install` in a +rebuilt image can resolve to something that 400s on every LLM call. + +Bump to a current floor and pin the image build. `openai>=1.0.0` is already +present (unused today — Track B uses it). + +**Files** +| File | Change | +|---|---| +| `AI_engine/requirements.txt` | modify — raise the `anthropic` floor | +| `AI_engine/Dockerfile` | modify — optional: `pip install` from a lockfile instead | +| `AI_engine/requirements-dev.txt` | check — may pin the same packages | + +### A4. `LLM_MODEL` default is a generation behind + +`core/llm.py:22` defaults to `claude-opus-4-8` ($5/$25 per MTok). `claude-opus-5-5` +is both cheaper ($4/$20) and more capable. One-line change; the registry's +per-agent `model` pin already overrides it (`registry.model(agent_id)`), so this +only moves the floor. + +**Files** +| File | Change | +|---|---| +| `AI_engine/core/llm.py:22` | modify — default `LLM_MODEL` | +| `AI_engine/core/llm.py:11` | modify — the docstring naming the default | +| `AI_engine/docker-compose.yml:45` | modify — `LLM_MODEL` fallback | +| `AI_engine/tests/test_llm.py` | check — may assert the old default | + +Note `core/llm.py:109` special-cases Haiku 4.5 (rejects adaptive thinking and +`effort`). Leave that branch alone; it is correct. + +--- + +## Track B — RAG decision memory + +The skeleton exists and is wired to nothing. Four pieces are already built: + +| Piece | Where | +|---|---| +| `context_embedding vector(1536)` on `agent_decisions` | `migrations/migrate.go:92` | +| ivfflat cosine index, `lists = 100` | `migrations/migrate.go:98` | +| `POST /internal/agent-decisions` accepts `context_embedding` | `controllers/agentDecisionController.go:23` | +| cosine kNN query + `PATCH /:id/outcome` | `agentDecisionController.go:74,120` | + +Nothing produces an embedding, nothing calls `/similar`, nothing records an +outcome. There are exactly **two** decision types to cover — +`assignment_failure` (`dispatch_agent.py:335`) and `stall_response` +(`exception_agent.py:456`). + +### B0. Three defects to fix before writing any new code + +**B0.1 — `CREATE EXTENSION vector` appears nowhere in the repo.** +`migrate.go:92` runs `ALTER TABLE agent_decisions ADD COLUMN … vector(1536)` and +logs failure *non-fatally*. If the extension is not installed on `logistics`, +both the column and the index silently fail and every retrieval 500s. + +```sql +SELECT extname, extversion FROM pg_extension WHERE extname = 'vector'; +``` + +If absent, add **before** line 92 in `Migrate()`: + +```go +if res := db.Exec(`CREATE EXTENSION IF NOT EXISTS vector`); res.Error != nil { + utils.Error("❌ pgvector extension unavailable — decision memory disabled", "error", res.Error) +} +``` + +Needs the `pgvector` extension available on the server and a role with rights to +create it. On a managed Postgres this may be an admin action, not a migration — +check before assuming. + +**B0.2 — `/similar` is a `GET` that requires a JSON body.** +`routes.go:594` registers it as `GET`, and `agentDecisionController.go:84` calls +`BodyParser` demanding an `embedding` array. nginx and most HTTP clients drop GET +bodies — and doormile is served *through* host nginx (see C4). Change to: + +```go +internal.Post("/agent-decisions/similar", controllers.FindSimilarDecisions) +``` + +No caller exists yet, so this breaks nothing. + +**B0.3 — `/similar` filters `WHERE outcome IS NOT NULL`, and nothing writes +outcomes.** Even with embeddings flowing it returns zero rows forever. **B2 is +not optional** — it is the half that makes retrieval worth anything. + +**Files** +| File | Change | +|---|---| +| `doormile_backend/migrations/migrate.go:92` | modify — add `CREATE EXTENSION` above the `ALTER TABLE` | +| `doormile_backend/routes/routes.go:594` | modify — `internal.Get` → `internal.Post` for `/agent-decisions/similar` | +| `doormile_backend/controllers/agentDecisionController.go:74` | modify — comment the method change; body parsing already correct | +| `doormile_backend/routes/routes_ai_registry_pg_test.go` | modify — add coverage for the POST shape | + +### B1. Embedding provider — decision required + +Anthropic has no embeddings endpoint, so this needs a second provider. + +| Option | Dims | Column change | Notes | +|---|---|---|---| +| **OpenAI `text-embedding-3-small`** | 1536 | **none** | ~$0.02/MTok. `openai>=1.0.0` already a dependency. Column was sized for it. | +| Voyage `voyage-3` | 1024 | yes | Anthropic-recommended; new key, new vendor. | +| Local `bge-small` / `MiniLM` | 384 | yes | Free, no egress, no key. +~400MB RAM, model in image, slower cold start. | + +**Recommendation: OpenAI `text-embedding-3-small`.** The schema already matches, +the dependency is already there, and the second-provider line is already crossed +(the playground runs on Groq). Revisit if data egress is a constraint — then take +the local model and migrate the column to `vector(384)`. + +### B2. Write path — `AI_engine/core/embeddings.py` + +One function, modelled on `core/decisions.py`'s fire-and-forget discipline: + +```python +async def embed(text: str) -> Optional[List[float]]: + """None on any failure. An embedding must never delay an agent's reaction.""" +``` + +Requirements: + +- **Embed the same dict `build_payload` already stores.** Serialise the `facts` + dict deterministically (sorted keys) so the embedded text and the stored + `context` cannot drift apart. Add a `_context_text(facts)` helper and test it + pure, the way `build_payload` is tested. +- Fail open — return `None`, log once, never raise into the agent path. +- Small LRU cache: repeated stalls on one booking produce near-identical facts. +- Gate on `EMBEDDINGS_ENABLED` **and** a registry skill flag, so it can be + switched off from Agent Studio without a redeploy + (`registry.skill_enabled(...)` already exists). + +Then extend `build_payload`/`record_decision` (`core/decisions.py:33,53`) to carry +`context_embedding`. Both call sites (`dispatch_agent.py:335`, +`exception_agent.py:456`) keep their signatures. + +**Acceptance:** after one stall, +`SELECT count(*) FROM agent_decisions WHERE context_embedding IS NOT NULL` > 0. + +**Files** +| File | Change | +|---|---| +| `AI_engine/core/embeddings.py` | **new** — `embed()`, `_context_text()`, LRU cache, fail-open | +| `AI_engine/core/decisions.py:33` | modify — `build_payload` carries `context_embedding` | +| `AI_engine/core/decisions.py:53` | modify — `record_decision` awaits the embed before posting | +| `AI_engine/config/system_config.py` | modify — `EMBEDDINGS_ENABLED`, provider key, model name | +| `AI_engine/docker-compose.yml` | modify — pass the embedding env through | +| `AI_engine/tests/test_embeddings.py` | **new** — `_context_text` determinism, fail-open returns `None` | +| `AI_engine/tests/test_registry_phase5.py:194` | modify — asserts the `record_decision` payload shape | + +Not touched: `agents/dispatch_agent.py:335` and `agents/exception_agent.py:456` +keep their call signatures — the embedding is added inside `decisions.py`, so +neither agent changes. + +### B3. Outcome loop — the part that makes retrieval useful + +`PATCH /internal/agent-decisions/:id/outcome` exists with no caller. Define, per +decision type, what "it worked" means. Starting proposal: + +| Type | Outcome = `success` when | `failure` when | +|---|---|---| +| `stall_response` | booking reaches `Delivered` within its SLA window after the decision | SLA breached, or cancelled | +| `assignment_failure` | booking gets an assignment within N minutes of the decision | still unassigned after N, or cancelled | + +Implement as a backend sweeper following the **established pattern** in +`internal/assignment/sweeper.go:88` — ticker, `recover()` per tick, and the Redis +lock that keeps one replica sweeping (`sweeper.go:104`). Wire it in `main.go` +beside `go assignment.StartPendingSweeper()` (`main.go:244`). + +Two gotchas from the existing code: +- Use `Receivedat`-style true instants, not `utils.DBNow` — `models/ai_runs.go:26` + documents that `DBNow` returns IST digits labelled UTC and is 5h30m off for + `timestamptz`. The same trap applies to `outcome_recorded_at`. +- Leave `outcome` NULL while undecided. `/similar` already treats NULL as + "no evidence yet", which is correct. + +**Acceptance:** rows acquire non-NULL `outcome` within one sweep interval of +their window closing, and `/insights` decision-outcome counts stop being all +`pending`. + +**Files** +| File | Change | +|---|---| +| `doormile_backend/internal/ai/outcomes/sweeper.go` | **new** — ticker + Redis lock, modelled on `internal/assignment/sweeper.go:88` | +| `doormile_backend/internal/ai/outcomes/rules.go` | **new** — the per-decision-type success/failure predicates | +| `doormile_backend/internal/ai/outcomes/sweeper_test.go` | **new** — interval, window, and both predicates | +| `doormile_backend/main.go:244` | modify — `go outcomes.StartOutcomeSweeper()` beside the pending sweeper | +| `doormile_backend/models/agentdecision.go` | modify — only if B6 adds `Tenantid` | + +Reference, not modified: `internal/assignment/sweeper.go:88-110` (the ticker + +`recover()` + Redis single-replica lock pattern to copy), +`models/ai_runs.go:26` (the `utils.DBNow` timezone trap to avoid). + +### B4. Read path — precedent in the prompt + +Before the LLM call in `exception_agent` / `dispatch_agent`, fetch the top-5 +similar **resolved** decisions and include them as precedent — "the last 5 +comparable situations and whether the action worked." + +- Feature-flag it on a registry skill so it is switchable from Agent Studio. +- Hard timeout (~300ms) with fail-open to today's prompt. The current behaviour + is the floor; this can only raise it. (`core/registry.py` already follows this + rule; `ragRouter.js:9-14` documents the same discipline on the console side.) +- Keep precedent **out** of the structured-output schema. It informs the prompt; + it must not become a field the model can invent. + +**Acceptance:** an eval run shows the decision quality moving. `AI_engine/evals/` +already has the harness and cases (`stall_cases.jsonl`, +`assignment_cases.jsonl`) — extend those rather than judging by eye. + +**Files** +| File | Change | +|---|---| +| `AI_engine/core/memory.py` | **new** — `recall(decision_type, embedding, k)` → POST `/internal/agent-decisions/similar`, timeout + fail-open | +| `AI_engine/core/llm.py:157` | modify — `decide_stall_response` accepts optional precedent | +| `AI_engine/core/llm.py:227` | modify — `decide_assignment_failure` accepts optional precedent | +| `AI_engine/agents/exception_agent.py:456` | modify — recall before the decide call | +| `AI_engine/agents/dispatch_agent.py:335` | modify — recall before the decide call | +| `AI_engine/tests/test_memory.py` | **new** — timeout fails open, empty recall changes nothing | +| `AI_engine/evals/stall_eval.py` | modify — run with and without precedent | +| `AI_engine/evals/assignment_eval.py` | modify — same | +| `AI_engine/evals/stall_cases.jsonl` | modify — cases where precedent should change the answer | +| `AI_engine/evals/assignment_cases.jsonl` | modify — same | +| `doormile_backend/internal/ai/registry/seed.go:114` | modify — seed a `recall_similar_decisions` read tool + the skill flag that gates B4 | + +The registry seed row matters: without it the feature cannot be switched off from +Agent Studio, which is the whole point of gating it on `registry.skill_enabled`. + +### B5. Hygiene + +- **`agent_decisions` has no retention.** `aiagentruns` purges at 30 days + (`telemetry/recorder.go:28,128`); decisions grow forever, and this is the table + retrieval scans. Decide a window — longer than 30 days, since old precedent is + the point. Suggest 180 days, or keep resolved rows and purge unresolved ones. +- **Re-tune `lists = 100`.** That is right for roughly 100k–1M rows. Below ~10k + it over-partitions and recall drops. Check `count(*)` once embeddings flow; + consider HNSW instead if the pgvector version supports it. + +**Files** +| File | Change | +|---|---| +| `doormile_backend/internal/ai/outcomes/sweeper.go` | modify — fold the decision purge into the same tick | +| `doormile_backend/migrations/migrate.go:98` | modify — index tuning, once row count is known | + +### B6. Tenant isolation — decide before B4 ships, not after + +`agent_decisions` has **no tenant column**. If retrieved precedent crosses +tenants, one client's operational history shapes decisions made for another. +Given that console logins are already unscoped on `tenantid` NULL +(`doormile-console-logins-unscoped`), this needs deciding up front. + +Recommendation: add `tenantid` to `agent_decisions`, have the engine populate it, +and filter in the `/similar` query. Cheap now, expensive after the table fills. + +**Files** +| File | Change | +|---|---| +| `doormile_backend/models/agentdecision.go` | modify — add `Tenantid *uint64` with an index | +| `doormile_backend/controllers/agentDecisionController.go:18` | modify — accept `tenant_id` on create | +| `doormile_backend/controllers/agentDecisionController.go:104` | modify — add `AND tenantid = ?` to the kNN query | +| `AI_engine/core/decisions.py:33` | modify — `build_payload` carries the tenant | +| `AI_engine/agents/exception_agent.py` · `dispatch_agent.py` | modify — source the tenant from the booking facts | +| `doormile_backend/routes/routes_ai_registry_pg_test.go` | modify — a cross-tenant recall must return nothing | + +Nullable, because the engine will not always know the tenant. Decide whether a +NULL tenant row is recallable by everyone or by no one — given +`doormile-console-logins-unscoped`, **by no one** is the safer default. + +--- + +## Track C — Kubernetes + +### C1. `AI_engine` is not in Kubernetes at all + +No manifest, no kustomization, no deploy script, and `docker-compose.yml` uses +`build: .` with no registry push. It runs on a VM by compose. + +**Prerequisite: the engine has no HTTP server in production mode.** +`main.py --production` starts no listener, so there is no liveness/readiness +target and no `/metrics`. Without it, Kubernetes can only restart on process +exit — a NATS-disconnected engine looks healthy forever. + +`fastapi` and `uvicorn` are already in `requirements.txt`. Add a small surface in +`production_mode()`: + +- `GET /healthz` — process alive (event loop responsive) +- `GET /readyz` — NATS connected **and** `registry.loaded` is true +- `GET /metrics` — optional; decisions recorded, tool calls, LLM failures + +Readiness must include `registry.loaded`, otherwise a pod that cannot reach the +backend serves traffic on env defaults while reporting healthy — exactly the A1 +failure mode, invisible again. + +Then: push the image to a registry (compose builds locally), and write +`manifests/doormile/ai-engine.yaml` as a `Deployment` (it is stateless; +`replicas: 1` to start — the agents are not yet idempotent across replicas, see +C6). + +**Files** +| File | Change | +|---|---| +| `AI_engine/core/health.py` | **new** — the aiohttp/FastAPI surface (`/healthz`, `/readyz`, `/metrics`) | +| `AI_engine/main.py:130` | modify — start the health server inside `production_mode()` beside `registry.run(...)` | +| `AI_engine/main.py` (`print_help`) | modify — document the health port | +| `AI_engine/Dockerfile` | modify — `EXPOSE` the health port | +| `AI_engine/docker-compose.yml` | modify — publish the port so compose and k8s behave alike | +| `AI_engine/tests/test_health.py` | **new** — `/readyz` is red while `registry.loaded` is false | +| `kubernetes/manifests/doormile/ai-engine.yaml` | **new** — Deployment + Service + probes + resources | + +`fastapi` and `uvicorn` are already in `requirements.txt` — no new dependency. +Readiness must check `registry.loaded` (`core/registry.py:45`), not just the +process, or A1's failure mode becomes invisible again. + +### C2. `doormile/` has no `kustomization.yaml` + +`alaska/`, `core/` and `nearle/` all have one. `doormile/` does not, and there is +no `deploy-doormile.sh` alongside `deploy-core-stack.sh` / `deploy-nearle-stack.sh`. +That is *why* it drifts. Add both. + +**Files** +| File | Change | +|---|---| +| `kubernetes/manifests/doormile/kustomization.yaml` | **new** — list miletruth, config, ai-engine, pdb | +| `kubernetes/deploy-doormile.sh` | **new** — copy the shape of `deploy-nearle-stack.sh` | +| `kubernetes/scripts/sync_manifests.py` | check — may need the new namespace registering | +| `kubernetes/docs/DEPLOY_CHECKLIST.md` | modify — add the doormile stack | + +Pattern to copy: `manifests/nearle/kustomization.yaml` + `deploy-nearle-stack.sh`. + +### C3. The `doormile` StatefulSet has no probes, resources or PDB + +`replicas: 3` with **no resource requests** means the scheduler can stack all +three on one node, and **no readinessProbe** means a pod receives traffic before +Postgres/Redis/NATS are connected. `core/` has `worker-pdb.yaml`; doormile has +nothing. + +Add `resources.requests`/`limits`, a readinessProbe and livenessProbe against the +backend's health route, and a PodDisruptionBudget (`minAvailable: 2`). + +Also: a `StatefulSet` for a stateless Go API is the wrong kind — it gives serial +rollouts and no benefit. Switching to `Deployment` is low-risk and makes deploys +faster. Not urgent; flagging because it is why rollouts feel slow. + +**Files** +| File | Change | +|---|---| +| `kubernetes/manifests/doormile/miletruth.yaml:23` | modify — `resources`, `readinessProbe`, `livenessProbe` | +| `kubernetes/manifests/doormile/doormile-pdb.yaml` | **new** — `minAvailable: 2`, copy `manifests/core/worker-pdb.yaml` | + +**No backend change needed** — the probe targets already exist and are correct: +`GET /api/v1/health` (`routes/routes.go:46`, unauthenticated, always 200) for +liveness, and `GET /api/v1/ready` (`routes/routes.go:50`) for readiness, which +already returns **503** when Postgres or Redis is unreachable +(`routes.go:68-70`). Point the probes at those; do not write new ones. + +Note `/ready` reports Redis GEO status without gating on it — deliberate, per the +comment at `routes.go:73`. A readinessProbe on `/ready` therefore will not pull a +pod out of service for a broken rider search, which is the intended behaviour. + +### C4. No ingress for `doormile` + +`nearle` and `alaska` are on `manifests/core/ingress-unified.yaml`. `doormile` is +NodePort 30830 plus host nginx (`conf/nginx-doormile.conf`) — half-migrated. This +also makes B0.2 (GET-with-body) a certainty rather than a risk. + +**Files** +| File | Change | +|---|---| +| `kubernetes/manifests/core/ingress-unified.yaml` | modify — add a `doormile` rule (needs a ReferenceGrant if it stays cross-namespace, cf. `manifests/nearle/nearle-reference-grant.yaml`) | +| `kubernetes/manifests/doormile/miletruth.yaml:91` | modify — `NodePort` → `ClusterIP` once the ingress serves it | +| `kubernetes/conf/nginx-doormile.conf` | modify — retire or repoint, **only after** the ingress is verified | + +Do these in that order. Flipping the Service type before the ingress works takes +the API offline. + +### C5. NATS is outside the cluster on two ports + +Backend uses `nats://66.116.226.161:4223`; `core-config.yaml:10` uses `:4222`. +Worth a line in `docs/ARCHITECTURE.md` on which port is which and why, before the +engine joins and needs to pick one. + +### C6. Decide replica safety before scaling the engine + +The telemetry recorder is already replica-safe (NATS queue group + +`uq_aiagentruns_agent_task`). The **agents** are not obviously so: `exception_agent` +has an in-process TTL dedup set (`exception_agent.py:339`), which is per-pod. Two +engine replicas would each decide on the same stall. Keep `replicas: 1` until +dedup moves to Redis. + +**Files** (only if scaling past 1 replica) +| File | Change | +|---|---| +| `AI_engine/agents/exception_agent.py:339` | modify — TTL set → Redis `SET NX EX` | +| `AI_engine/tests/test_stall_dedup.py` | modify — covers the current in-process behaviour | +| `kubernetes/manifests/doormile/ai-engine.yaml` | modify — raise `replicas` | + +C5 (the NATS port note) is documentation only: `kubernetes/docs/ARCHITECTURE.md`. + +--- + +## Track D — Executor backlog + +9 of 22 seeded tools are marked `REVIEW ONLY … No executor` in +`internal/ai/registry/seed.go`. The registry is honest about it and the console +renders them disabled. This is the feature list, in value order. + +### D1. `assign_riders` — needs an admin auto-assign route (not just auth) + +**Correcting an earlier assumption:** this is *not* a free auth fix. +`actions.js:96-107` already explains why — `POST /admin/bookings/:id/assign-miler` +(`routes.go:413` → `adminController.go:2965`) requires a **chosen rider per +booking** (`{mileruserid}`), and a finding does not pick one. The hub route that +*does* pick (`POST /hub/bookings/:id/auto-assign`, `routes.go:520`) is behind +`HubStaffAuth` and 403s for every console login. + +**The cleanest route is an admin batch-assign**, better than the per-booking +auto-assign first considered. `HubBatchAssign` (`controllers/hubController.go:1963`) +already does exactly what a finding needs: it takes `bookingids[]`, picks riders +via Redis GEO + scoring, and commits server-side in one call. Its only hub-specific +parts — `c.Locals("hubid")` and `hubPincodePrefix(hubID)` (`hubController.go:1964-1966`) +— are used **solely as a fallback when `bookingids` is empty** (`hubController.go:1981-1985`). +The console always passes explicit ids, so that branch never runs. + +So: extract the body into a shared helper taking `(bookingIDs, capPerRider, actorID, scopeFn)` +and have both the hub route and a new admin route call it. `scopeBookingsToOwnTenant` +(`hubController.go:1986`) already works for admin logins. + +The console side is then nearly free — `batchAssignBookings` already exists at +`src/api/doormile/endpoints.js:554` and already sends `{bookingids, max_per_rider}`. +It just points at the hub URL that 403s. One URL change. + +Option (b), having the skill pick a rider via `nearby_milers` and call the +existing `assign-miler`, is worse: more console work and it puts solver logic in +the browser. + +**Files** +| File | Change | +|---|---| +| `doormile_backend/controllers/hubController.go:1963` | modify — extract the shared assign helper out of `HubBatchAssign` | +| `doormile_backend/controllers/adminController.go` | **add** `AdminBatchAssign` calling that helper | +| `doormile_backend/routes/routes.go:413` | modify — register `adminAuth.Post("/bookings/batch-assign", …)` | +| `krow_talent_app/src/api/doormile/endpoints.js:554` | modify — `/hub/bookings/batch-assign` → `/admin/bookings/batch-assign` | +| `krow_talent_app/src/lib/assistant/agent/actions.js:122` | modify — add the `assignMiler` executor | +| `krow_talent_app/src/lib/assistant/agent/actions.js:96-107` | modify — delete the "deliberately NOT an executor" note | +| `krow_talent_app/tests/lib/agentActions.test.js` | modify — asserts the current executor set | +| `doormile_backend/internal/ai/registry/seed.go` | modify — `assign_riders` description stops saying REVIEW ONLY | + +Check before starting: `endpoints.js:547-553` warns that Doormile-native batch +assign commits with no preview/reconcile step and leaves multi-stop riders +unsequenced. An agent-proposed assignment firing straight to commit is a +behaviour decision, not just a wiring one — confirm that is wanted. + +This takes the console from 1 working verb to 2 and makes the highest-severity +SLA finding actionable. + +### D2. `alert_low_battery_rider` — nearly free + +`seed.go` already targets `POST /admin/milers/:id/notify`, which is the endpoint +`notify_riders` already uses successfully. This is a message-text change and an +`EXECUTORS` entry, not a new capability. + +**Files** +| File | Change | +|---|---| +| `krow_talent_app/src/lib/assistant/agent/actions.js:122` | modify — add the `alertLowBatteryRider` executor | +| `krow_talent_app/src/lib/assistant/skills/definitions/RiderBatterySafetySkill.js` | check — confirm the proposal carries `milerId` | +| `krow_talent_app/tests/lib/agentActions.test.js` | modify — same assertion as D1 | +| `doormile_backend/internal/ai/registry/seed.go` | modify — drop REVIEW ONLY from the description | + +No backend change. `notifyMiler` already exists at +`src/api/doormile/endpoints.js:389` → `POST /admin/milers/:id/notify`, which is +the endpoint `seed.go` already names as the target. + +### D3. The rest need endpoints that do not exist + +`enforce_otp_verification`, `dispatch_hub_idle_parcels`, `trigger_auto_dispatch`, +`enforce_cash_handoff`, `rebalance_riders` — all marked `Target: "none yet"`. +Each is a product decision first. Not in this phase. + +--- + +## Two more loose ends + +- **`src/lib/agentNetwork.js` is a hand-maintained snapshot** dated 16–20 Sep, and + the file says so honestly. Once C1 gives the engine an HTTP surface, add + `GET /agents/status` and swap the source. The file is deliberately shaped like + that response, so it is a change of source, not a rewrite. +- **`src/lib/assistant/ragRouter.js` points at a `services/ai` sidecar that does + not exist in any repo.** `VITE_AI_URL` appears nowhere, so `isRagEnabled()` is + permanently false and the module is dead code. **Do not conflate this with + Track B** — ragRouter is semantic *intent routing* for the console assistant, + not decision memory. Lower value. Leave it dormant (it is correctly fail-open) + or decide to build the sidecar as its own piece of work. + +--- + +## Consolidated file manifest + +**14 new files, 46 modified, across 4 repos.** Per-item detail is in the tracks above. + +### `doormile_backend` — 4 new, 17 modified + +| File | New? | Items | +|---|---|---| +| `internal/ai/outcomes/sweeper.go` | **new** | B3, B5 | +| `internal/ai/outcomes/rules.go` | **new** | B3 | +| `internal/ai/outcomes/sweeper_test.go` | **new** | B3 | +| `migrations/migrate.go` | | B0.1 (:92), B5 (:98) | +| `routes/routes.go` | | B0.2 (:594), D1 (:413) | +| `controllers/agentDecisionController.go` | | B0.2 (:74), B6 (:18, :104) | +| `controllers/hubController.go` | | D1 (:1963 — extract helper) | +| `controllers/adminController.go` | | D1 (**add** `AdminBatchAssign`) | +| `models/agentdecision.go` | | B6 | +| `internal/ai/registry/seed.go` | | B4 (:114), D1, D2 | +| `main.go` | | B3 (:244) | +| `routes/routes_ai_registry_pg_test.go` | | B0.2, B6 | + +### `AI_engine` — 5 new, 20 modified + +| File | New? | Items | +|---|---|---| +| `core/embeddings.py` | **new** | B2 | +| `core/memory.py` | **new** | B4 | +| `core/health.py` | **new** | C1 | +| `tests/test_embeddings.py` | **new** | B2 | +| `tests/test_memory.py` · `tests/test_health.py` | **new** | B2, C1 | +| `core/decisions.py` | | B2 (:33, :53), B6 | +| `core/llm.py` | | A4 (:11, :22), B4 (:157, :227) | +| `core/registry.py` | | — read-only (`loaded` consumed by C1) | +| `agents/exception_agent.py` | | B4 (:456), B6, C6 (:339) | +| `agents/dispatch_agent.py` | | B4 (:335), B6 | +| `main.py` | | C1 (:130, `print_help`) | +| `config/system_config.py` | | B2 | +| `requirements.txt` · `Dockerfile` · `docker-compose.yml` | | A3, A4, C1 | +| `evals/stall_eval.py` · `assignment_eval.py` · both `.jsonl` | | B4 | +| `tests/test_registry_phase5.py` · `test_llm.py` · `test_stall_dedup.py` | | B2, A4, C6 | + +### `kubernetes` — 5 new, 7 modified + +| File | New? | Items | +|---|---|---| +| `manifests/doormile/doormile-config.yaml` | **new** | A1 | +| `manifests/doormile/ai-engine.yaml` | **new** | C1, C6 | +| `manifests/doormile/kustomization.yaml` | **new** | C2 | +| `manifests/doormile/doormile-pdb.yaml` | **new** | C3 | +| `deploy-doormile.sh` | **new** | C2 | +| `manifests/doormile/miletruth.yaml` | | A1, A2, C3 (:23), C4 (:91) | +| `manifests/core/ingress-unified.yaml` | | C4 | +| `conf/nginx-doormile.conf` | | C4 (last) | +| `scripts/sync_manifests.py` | | C2 | +| `docs/DEPLOY.md` · `DEPLOY_CHECKLIST.md` · `ARCHITECTURE.md` | | A2, C2, C5 | + +### `krow_talent_app` — 0 new, 4 modified + +The console barely changes. Everything it needs already exists. + +| File | Items | +|---|---| +| `src/api/doormile/endpoints.js` | D1 (:554 — one URL) | +| `src/lib/assistant/agent/actions.js` | D1 (:96-107, :122), D2 (:122) | +| `tests/lib/agentActions.test.js` | D1, D2 | +| `src/lib/assistant/skills/definitions/RiderBatterySafetySkill.js` | D2 — check only | + +Untouched on purpose: `src/lib/agentNetwork.js` (until C1 ships an endpoint) and +`src/lib/assistant/ragRouter.js` (dormant, out of scope — see loose ends). + +### Files deliberately not touched + +- `src/lib` / `src/utils` duplicate trees — both have live importers, do not consolidate. +- `AI_engine/core/tool_registry.py` — the 2-vs-22 tool gap is a separate decision, not this phase. +- `AI_engine/customer_portal/`, `dashboard/` — not on any path this phase touches. + +## Decisions needed before coding + +1. **Embedding provider** — OpenAI 1536 (no column change), Voyage 1024, or local + 384? (B1) +2. **Is `pgvector` installed on `logistics`?** If creating extensions needs an + admin, that is a prerequisite, not a migration. (B0.1) +3. **Outcome definitions** — are the two in B3 right, and what is N for + `assignment_failure`? +4. **`tenantid` on `agent_decisions`** — add it now, or accept cross-tenant + precedent? (B6) +5. **`assign_riders`** — route (a) admin auto-assign, or (b) console picks the + rider? (D1) +6. **Decision retention window** — 180 days, or keep-resolved-purge-unresolved? (B5) + +## Sequencing + +``` +A1 ──> A3, A4 ──┬──> B0 ──> B2 ──> B3 ──> B4 ──> B5, B6 + │ + └──> C1(health) ──> C1(manifest) ──> C2 ──> C3 ──> C4 +A2 (independent, before A1 adds more secrets to git) +D1, D2 (independent of everything above) +``` + +**A1 first and alone.** Until the internal key is set, the engine is not talking +to the backend, so every Track B acceptance check would fail for the wrong +reason. Run the `kubectl` check in A1 before writing any code — if git does not +match the cluster, that changes the shape of Track C. + +Cheapest real step up a level, once A1 is in: **D1 + D2.** Two executors, no new +LLM work, and it moves autonomy off the floor. + +--- + +## Standing constraints + +- Nothing in this plan is committed, pushed, or deployed without being asked. +- No migrations run against a real database without being asked. +- No files deleted or untracked without per-file approval. +- `src/lib` and `src/utils` in the console **both** have live importers — neither + tree is dead, do not consolidate them as part of this work. diff --git a/src/api/doormile/endpoints.js b/src/api/doormile/endpoints.js index f969284..d294301 100644 --- a/src/api/doormile/endpoints.js +++ b/src/api/doormile/endpoints.js @@ -545,14 +545,27 @@ export const assignMilerToBooking = async (id, data) => { return response.data; }; -// Doormile-native bulk assign: picks real milers via Redis GEO + AI scoring, no -// external solver involved. Commits server-side in this one call — unlike the -// workolik solver flow, there's no separate preview/reconcile/commit step. -// Sequencing (POST /optimization/doormile/sequence) is not deployed yet, so -// riders with more than one stop come back unsequenced (step: 0) — see -// doormile-flow.md's State table. +// Doormile-native bulk assign: greedy nearest on-duty rider by haversine, +// capped per rider, committed server-side in this one call — there is no +// separate preview/reconcile/commit step. +// +// ADMIN endpoint, not the hub one. It used to point at +// POST /hub/bookings/batch-assign, which sits behind HubStaffAuth and 403s for +// every token this console issues — so this function could never succeed from +// here. /admin/bookings/batch-assign runs the SAME solver +// (controllers/batchAssignService.go) with admin auth, which is what makes the +// ops layer's assignMiler proposal executable at all. +// +// Unlike the hub route, an empty bookingids is refused rather than meaning +// "clear the queue": an admin login has no hub bound, so empty would mean every +// pending booking in the system. +// +// Stop sequencing runs after the assignments commit and is best-effort. It +// needs ROUTE_OPTIMIZER_URL set in the cluster; without it riders with more +// than one stop come back unsequenced (step: 0) — see doormile-flow.md's State +// table. export const batchAssignBookings = async (bookingIds, maxPerRider = 5) => { - const response = await doormileAxios.post('/hub/bookings/batch-assign', { + const response = await doormileAxios.post('/admin/bookings/batch-assign', { bookingids: bookingIds, max_per_rider: maxPerRider }); @@ -860,6 +873,44 @@ export const runAiPlayground = async (body) => { return response.data.data; }; +/* Skill findings. The console's rule output, reported so it survives the render + that produced it — see lib/assistant/agent/findingReport.js for why. Called + fire-and-forget: a failed report must never surface to an operator looking at + a stalled parcel. */ +export const reportFindings = async (payload) => { + const response = await doormileAxios.post('/admin/ai/findings', payload); + return response.data; +}; + +export const reportFindingActed = async (fingerprint, result) => { + const response = await doormileAxios.post( + `/admin/ai/findings/${encodeURIComponent(fingerprint)}/acted`, { result } + ); + return response.data; +}; + +export const getAiFindings = async ({ days, skill, limit } = {}) => { + const response = await doormileAxios.get(`/admin/ai/findings${buildQuery({ days, skill, limit })}`); + return response.data.data; +}; + +/** Per skill: how often it fires, how long findings stay open, how often anyone + acts, and whether acting cleared them. "Acted and cleared" vs "cleared on + its own" is what separates a skill that helps from one that narrates. */ +export const getAiFindingStats = async (days = 30) => { + const response = await doormileAxios.get(`/admin/ai/findings/stats${buildQuery({ days })}`); + return response.data.data; +}; + +/** Expected pickups per zone with the STAFFING GAP against riders on duty. + A bare forecast number is not actionable — nobody knows whether 42 is fine. + The gap is: "641 expects 42, has 6 riders at 5 stops each, short by 12". + Produced daily by AI_engine/prediction and stored by the backend. */ +export const getDemandForecast = async (days = 7) => { + const response = await doormileAxios.get(`/admin/ai/forecast/demand${buildQuery({ days })}`); + return response.data.data; +}; + /** Recent agent decisions, newest first. `before` is the last id of the previous page. */ export const getAiDecisions = async ({ type, before, limit } = {}) => { const response = await doormileAxios.get(`/admin/ai/decisions${buildQuery({ type, before, limit })}`); diff --git a/src/components/doormile/AgentOperationsBanner.jsx b/src/components/doormile/AgentOperationsBanner.jsx index 84d3da8..33b56f6 100644 --- a/src/components/doormile/AgentOperationsBanner.jsx +++ b/src/components/doormile/AgentOperationsBanner.jsx @@ -6,6 +6,7 @@ import SlaRemediationCard from './SlaRemediationCard'; import { scanBookings } from '@/lib/assistant/scan'; import { toAgentRows } from '@/lib/assistant/agent/normalise'; import { AgentFactory } from '@/lib/assistant/agent/AgentFactory'; +import { reportScan, reportActed } from '@/lib/assistant/agent/findingReport'; import { SkillRegistry } from '@/lib/assistant/skills/SkillRegistry'; import { wallClockNow } from '@/lib/assistant/agent/signals'; import { executeProposal, canExecuteProposal } from '@/lib/assistant/agent/actions'; @@ -62,7 +63,12 @@ export default function AgentOperationsBanner({ const scan = await scanBookings(); const agentRows = toAgentRows(scan?.rows); const agent = AgentFactory.synthesizeDefaultAgent(); - return agent.evaluateTelemetry(agentRows, wallClockNow()); + const findings = agent.evaluateTelemetry(agentRows, wallClockNow()); + // Reported AFTER evaluating and deliberately NOT awaited: persistence is + // observability and must never sit in front of what the operator sees. + // reportScan never throws. + reportScan(findings); + return findings; }, staleTime: 30_000, refetchInterval: 60_000, @@ -81,6 +87,10 @@ export default function AgentOperationsBanner({ const handleRemediate = useCallback( async (finding) => { const result = await executeProposal(finding); + // Recorded before the throw: a failed action is exactly the outcome worth + // keeping, and recording only successes would make every skill look + // perfect. + reportActed(finding, result); if (!result?.ok) { throw new Error(result?.message || 'The action did not complete.'); } diff --git a/src/lib/agentNetwork.js b/src/lib/agentNetwork.js index ab467ce..c80e062 100644 --- a/src/lib/agentNetwork.js +++ b/src/lib/agentNetwork.js @@ -165,7 +165,7 @@ export const AGENTS = [ load: 0, blurb: 'Builds delivery routes from zones and available hubs. Ten-minute cache.', detail: - 'Sequences waypoints into routes using zone membership and hub availability. This is the piece the Go backend has no equivalent for — HubBatchAssign decides who gets a booking, never what order a rider runs their stops in.', + 'Sequences waypoints into routes using zone membership and hub availability. SIMULATION: its zones and traffic patterns are hard-coded in _init_zones()/_init_traffic_patterns() and it makes no backend or routing-API call at all. Real stop ordering is done by internal/routing against the Valhalla-backed Route Optimization API, not here. The registry records it with status simulation.', subscribes: [], publishes: 'Nothing on the bus.', api: 'No.', @@ -186,7 +186,7 @@ export const AGENTS = [ load: 0, blurb: 'Hub operations — transit records, capacity, what is sitting where.', detail: - 'Tracks inventory at each hub and the transit records moving between them, and answers capacity questions about whether a hub can absorb more volume.', + 'Tracks inventory at each hub and the transit records moving between them, and answers capacity questions about whether a hub can absorb more volume. SIMULATION: its hubs are 8 hard-coded entries in _init_hubs() (DL-HUB-01 and friends, with invented connected_hubs graphs) — not the hubs table. The registry records it with status simulation.', subscribes: [], publishes: 'Nothing on the bus.', api: 'No.', @@ -207,7 +207,7 @@ export const AGENTS = [ load: 0, blurb: 'Vehicles: status, capacity and live tracking.', detail: - "Holds vehicle state and assignment, and answers which vehicle can carry a given load. It overlaps with the Go backend's own Vehicle model, which is the live source today.", + "Holds vehicle state and assignment, and answers which vehicle can carry a given load. It overlaps with the Go backend's own Vehicle model, which is the live source today. SIMULATION: its fleet is 19 vehicles hard-coded in _init_fleet() across Delhi, Mumbai, Bangalore, Hyderabad, Pune and Kolkata — not the vehicles table, and not the cities Doormile serves. The registry records it with status simulation for this reason.", subscribes: [], publishes: 'Nothing on the bus.', api: 'No.', diff --git a/src/lib/assistant/agent/actions.js b/src/lib/assistant/agent/actions.js index e17123f..5d3783f 100644 --- a/src/lib/assistant/agent/actions.js +++ b/src/lib/assistant/agent/actions.js @@ -17,7 +17,7 @@ // • A partial success is a partial success. Notifying six riders out of eight // is reported as six out of eight, with the two failures named. -import { getMilers, buildMilerLookup, notifyRider } from '@/api/doormile'; +import { getMilers, buildMilerLookup, notifyRider, batchAssignBookings } from '@/api/doormile'; /** A stable, human-facing reference for a row. */ const ref = (row) => row?.orderid || row?.bookingno || (row?.bookingid ? `#${row.bookingid}` : '—'); @@ -86,6 +86,9 @@ const executeNotifyRider = async (scope, message) => { return { ok: notified.length > 0, + // Same contract as executeAssignMiler: say whether this was partial rather + // than leaving every caller to work it out from a different field. + partial: notified.length > 0 && unreachable.length > 0, message: parts.join(' ') || 'Nothing was sent.', notified, unreachable, @@ -93,18 +96,86 @@ const executeNotifyRider = async (scope, message) => { }; }; -// ---- assignMiler: deliberately NOT an executor -------------------------------- +// ---- assignMiler ------------------------------------------------------------ // -// On the branch this called batchAssignBookings → POST /hub/bookings/batch-assign. -// That route sits behind middlewares.HubStaffAuth, which refuses every token -// whose role is not 6 (hub staff) — so from this console, where every login is -// admin/manager/executive, it would 403 on every click. Shipping it would have -// put a button on the Exceptions banner that could never succeed. +// This WAS review-only, and the note explaining why is worth keeping because +// it names the actual constraint: // -// Findings that propose `assignMiler` therefore render "Review only". The -// admin route that could back it, POST /admin/bookings/:id/assign-miler, needs -// a chosen rider per booking, which a finding does not pick. Wiring this is a -// backend decision (an admin batch-assign route), not a console one. +// On the branch this called batchAssignBookings → POST /hub/bookings/batch-assign. +// That route sits behind middlewares.HubStaffAuth, which refuses every token +// whose role is not 6 (hub staff) — so from this console, where every login is +// admin/manager/executive, it would 403 on every click. The admin route that +// could back it, POST /admin/bookings/:id/assign-miler, needs a chosen rider +// per booking, which a finding does not pick. +// +// The resolution was the backend decision that note called for: +// POST /admin/bookings/batch-assign now runs the same solver as the hub route +// (controllers/batchAssignService.go) under admin auth, so the console gets the +// solver that PICKS riders rather than one that needs them named. +// +// ⚠ This commits. There is no preview/reconcile step on either batch-assign +// route, so the proposal gate in the UI is the only thing between a finding and +// a real assignment. canExecuteProposal must stay the gate, and the card must +// keep requiring an explicit click — do not make this run automatically. +const executeAssignMiler = async (scope) => { + const rows = (scope || []).filter((r) => r?.bookingid); + if (!rows.length) { + return { ok: false, message: 'None of these findings carries a booking to assign.', sourceCalls: [] }; + } + + const bookingIds = [...new Set(rows.map((r) => Number(r.bookingid)))]; + const target = 'POST /admin/bookings/batch-assign'; + + try { + const res = await batchAssignBookings(bookingIds); + // The backend reports per booking, and a partial success is a partial + // success — the same rule executeNotifyRider follows. Reporting "assigned" + // when four of nine landed sends a dispatcher away from five parcels that + // still have nobody. + const data = res?.data || res || {}; + const assigned = Number(data.assigned ?? 0); + const skipped = Number(data.skipped ?? 0); + const sequenced = Number(data.riderssequenced ?? 0); + const reasons = (data.results || []) + .filter((r) => !r?.assigned && r?.reason) + .map((r) => `${r.bookingno || `#${r.bookingid}`} (${r.reason})`); + + const parts = []; + if (assigned) parts.push(`Assigned ${assigned} order${assigned === 1 ? '' : 's'}.`); + if (skipped) parts.push(`Could not assign ${skipped}: ${reasons.join('; ') || 'no reason given'}.`); + // Stop order needs ROUTE_OPTIMIZER_URL configured. Saying nothing when it + // is zero would leave the operator assuming routes were ordered. + if (assigned && !sequenced) parts.push('Stops were not sequenced (route optimiser unavailable).'); + else if (sequenced) parts.push(`Sequenced ${sequenced} rider${sequenced === 1 ? '' : 's'}.`); + + return { + ok: assigned > 0, + // Declared, not inferred. reportActed used to detect a partial run by + // looking for an `unreachable` array, which only the notify executor + // returns — so a batch that assigned two of three was recorded as a + // clean success. The message said "partial", the telemetry said "ok", + // and the measurement the findings table exists for was wrong. + partial: assigned > 0 && skipped > 0, + message: parts.join(' ') || 'Nothing was assigned.', + sourceCalls: [{ + name: 'batchAssignBookings', + target, + status: assigned > 0 ? 'complete' : 'error', + stats: `${assigned} assigned, ${skipped} skipped, ${sequenced} sequenced`, + ...(assigned === 0 ? { errorMessage: reasons.join('; ') || 'no rider under capacity' } : {}) + }] + }; + } catch (err) { + return { + ok: false, + message: err?.message || 'The assignment did not complete.', + sourceCalls: [{ + name: 'batchAssignBookings', target, status: 'error', + errorMessage: err?.message || 'assignment failed' + }] + }; + } +}; // ---- registry --------------------------------------------------------------- @@ -127,8 +198,38 @@ const EXECUTORS = { finding?.severity === 'critical' ? 'Urgent: this delivery needs attention now. Please check the app.' : 'Please check this delivery in the app when you can.' - ) - // assignMiler: none — see the note above. Its findings render "Review only". + ), + + // alert_low_battery_rider: the same endpoint notifyRider already uses, so + // this is a message, not a new capability. The registry seed names + // POST /admin/milers/:id/notify as its target and it was marked REVIEW ONLY + // only because nothing had been wired to it. + // + // The tool key is snake_case here, unlike notifyRider above, because + // RiderBatterySafetySkill.js:84 emits `alert_low_battery_rider` and this map + // is keyed on the verb a finding actually carries — not on a convention. + // Renaming the skill's verb to match would be the tidier fix and a wider + // change; it is not worth touching a shipped skill's output for. + alert_low_battery_rider: (finding) => + executeNotifyRider( + finding?.proposal?.scope, + 'Low battery: please charge now, or report to the nearest hub if you cannot.' + ), + + assignMiler: (finding) => executeAssignMiler(finding?.proposal?.scope), + + // trigger_auto_dispatch: LateDispatchSkill proposes "auto-assign orders that + // have waited too long for dispatch", which is exactly what batch-assign + // does. seed.go marked it Target: "none yet" because no route picked riders + // for an admin login; /admin/bookings/batch-assign now does, so this needed + // no new capability — only the realisation that it is the same action under + // a different name. + // + // Deliberately NOT given its own executor: two code paths assigning riders + // would be a second definition of the same write, and the codebase already + // carries three (internal/assignment's AI path, AssignMilerToBooking, and + // the batch solver). One more would be the one that drifts. + trigger_auto_dispatch: (finding) => executeAssignMiler(finding?.proposal?.scope) }; /** Whether a finding's proposal can actually be carried out. */ diff --git a/src/lib/assistant/agent/findingReport.js b/src/lib/assistant/agent/findingReport.js new file mode 100644 index 0000000..e43d66d --- /dev/null +++ b/src/lib/assistant/agent/findingReport.js @@ -0,0 +1,150 @@ +// ==============================|| Doormile AI — finding persistence ||============================== // +// +// Reports what the rule skills noticed to the backend, so it survives the +// render that produced it. +// +// The eight skills run here, in the operator's browser, on a 60-second React +// Query interval — and their output was discarded every cycle. 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 single time? +// • Did the proposal an operator carried out actually clear the finding? +// +// The third is the one worth having. AgentOperationsBanner already re-runs its +// scan after executing a proposal precisely to see whether the finding +// disappears — that evidence existed for one render and then vanished. +// +// ─── Rules this module follows ─────────────────────────────────────────────── +// +// • It is FIRE-AND-FORGET and never throws. Reporting is observability; the +// briefing must render identically whether or not the POST lands. A +// dispatcher looking at a stalled parcel does not care that telemetry is +// down, and must not be shown an error about it. +// +// • It sends the CLEARED set too. Only the console knows the full set it +// evaluated, so only it can tell the backend which fingerprints are gone. +// The backend cannot distinguish "resolved" from "the operator closed the +// tab". +// +// • It sends no personal data. Scope goes as booking ids only — no names, +// phones or addresses. The findings table is for measuring the skills, not +// a second copy of the bookings. + +import { reportFindings, reportFindingActed } from '@/api/doormile'; + +/** + * The identity of a finding across polls. + * + * Deliberately only the skill and its scope's booking ids, sorted. NOT severity + * and NOT counts: both drift while the underlying problem is unchanged (an + * ageing SLA breach climbs from warning to critical), and including them would + * make every escalation look like a brand-new finding and reset the "how long + * has this been open" measurement — which is the single most useful thing the + * table records. + * + * Sorted because scope order is not stable across scans. + */ +export const fingerprintOf = (finding) => { + const skill = finding?.skillId || finding?.skill || 'unknown'; + // The PROPOSED ACTION is part of the identity. + // + // One skill legitimately raises several findings over the same booking set — + // SlaGuardian emits both "notify the assigned riders" and "assign these + // orders" for the same three parcels. Keyed on skill + scope alone those + // collide, and because the fingerprint is the upsert key the second finding + // silently overwrites the first: one of the two disappears from the table and + // from every measurement built on it. + // + // Caught by running the real Exceptions page against a mock backend; the unit + // tests used a distinct skill per fingerprint and never produced a collision. + const tool = finding?.proposal?.tool || 'none'; + const ids = (finding?.proposal?.scope || []) + .map((r) => r?.bookingid) + .filter((id) => id != null) + .map(Number) + .sort((a, b) => a - b); + return `${skill}:${tool}:${ids.join(',')}`; +}; + +const toItem = (finding) => ({ + skillid: finding?.skillId || finding?.skill || 'unknown', + fingerprint: fingerprintOf(finding), + severity: finding?.severity || 'info', + title: String(finding?.title || finding?.headline || '').slice(0, 300), + proposaltool: finding?.proposal?.tool || '', + // Ids only. See the no-personal-data rule above. + bookingids: (finding?.proposal?.scope || []) + .map((r) => r?.bookingid) + .filter((id) => id != null) + .map(Number), + tenantid: Number(localStorage.getItem('tenantid')) || null +}); + +// Fingerprints seen on the previous scan, so this one can report what went +// away. Module-level rather than component state: the banner remounts on +// navigation and a remount must not look like every finding clearing at once. +let previous = new Set(); + +/** + * Report one scan's findings. Returns nothing and never rejects. + * + * Call AFTER the findings are rendered, not before — this must never sit in + * front of what the operator sees. + */ +export const reportScan = async (findings) => { + try { + const items = (findings || []).map(toItem).filter((i) => i.fingerprint && i.skillid); + const current = new Set(items.map((i) => i.fingerprint)); + const cleared = [...previous].filter((fp) => !current.has(fp)); + + // Nothing open and nothing cleared is the common case on a quiet board; + // skip the round trip entirely. + if (!items.length && !cleared.length) { + previous = current; + return; + } + + await reportFindings({ findings: items, cleared }); + previous = current; + } catch { + // Silent by design. A failed report is not something an operator can act + // on, and the previous set is deliberately NOT updated — so the next scan + // retries the same clear set rather than losing it. + } +}; + +/** + * Record that an operator carried out a proposal, and how it went. + * + * `result` mirrors what the executors actually return: a partial success is a + * partial success. Flattening six-riders-notified-out-of-eight to "ok" would + * lose exactly the distinction actions.js preserves. + */ +export const reportActed = async (finding, executionResult) => { + try { + const fp = fingerprintOf(finding); + if (!fp) return; + let result = 'failed'; + if (executionResult?.ok) { + // `partial` is declared by the executor. The fallback covers an executor + // that predates the field; without the declaration a batch-assign that + // skipped a booking reported as a clean success, because `unreachable` + // is a notify-only field. + const partial = typeof executionResult.partial === 'boolean' + ? executionResult.partial + : (Array.isArray(executionResult?.unreachable) && executionResult.unreachable.length > 0); + result = partial ? 'partial' : 'ok'; + } + await reportFindingActed(fp, result); + } catch { + // Same reasoning as reportScan: the action itself already happened and was + // reported to the operator. Losing its telemetry must not surface as a + // failure of the action. + } +}; + +/** Tests only — the module-level previous set would otherwise leak between them. */ +export const __resetForTests = () => { + previous = new Set(); +}; diff --git a/src/lib/clientOnboarding.js b/src/lib/clientOnboarding.js index 96d9b18..9837355 100644 --- a/src/lib/clientOnboarding.js +++ b/src/lib/clientOnboarding.js @@ -38,6 +38,29 @@ export function generateClientPassword(length = 14) { } /** Client-side mirror of the server's validation, for inline errors. */ +/** + * What a client ships. The SAME eight values the backend prices against + * (constants.DeliveryCategories) — a tenant whose category is not a pricing + * category cannot be priced, so this must not drift from that list. + */ +export const DELIVERY_CATEGORIES = [ + 'General', 'Documents', 'Electronics', 'Clothing', + 'Fragile', 'Medical', 'Automotive', 'Food' +]; + +/** + * Whether a return journey makes sense for what this client ships. + * + * Food is the exception: a meal that comes back is waste, not inventory. + * Mirrors constants.ReverseLogisticsAllowed, which ENFORCES this in + * InitiateConsignmentRTO — this copy only decides what the form shows, and + * hiding a control is a courtesy, not a rule. + * + * Unknown or empty allows returns, matching the server: the field is new, and + * new information must not withdraw a capability existing clients already use. + */ +export const reverseLogisticsAllowed = (category) => category !== 'Food'; + export function validateOnboarding(form) { const errors = {}; const phone = String(form.phone || '').replace(/\D/g, ''); @@ -46,6 +69,12 @@ export function validateOnboarding(form) { if (!/^[^\s@<>]+@[^\s@<>]+\.[^\s@<>]+$/.test(String(form.email || '').trim())) errors.email = 'Enter a valid email address'; if (!/^[6-9]\d{9}$/.test(phone)) errors.phone = 'Enter a 10-digit mobile number'; if (!form.applocationid) errors.applocationid = "Choose the client's operating city"; + // Required, matching the server. Not defaulted to General: the category sets + // the client's pricing AND whether their parcels can be returned, so + // guessing it decides both on the operator's behalf without asking. + if (!DELIVERY_CATEGORIES.includes(String(form.deliverycategory || ''))) { + errors.deliverycategory = 'Choose what this client delivers'; + } const pw = String(form.password || ''); if (pw.length < 8) errors.password = 'At least 8 characters'; else if (pw.length > 72) errors.password = 'At most 72 characters'; diff --git a/src/pages/doormile/clients/ClientLoginDialogs.jsx b/src/pages/doormile/clients/ClientLoginDialogs.jsx index ee2b69b..841da09 100644 --- a/src/pages/doormile/clients/ClientLoginDialogs.jsx +++ b/src/pages/doormile/clients/ClientLoginDialogs.jsx @@ -4,7 +4,14 @@ import { Alert, Button, Field, Input, Modal, Switch } from '@/components/ds'; import { inputVariants } from '@/components/ui/input'; import { useDeleteOnboardedClient, useUpdateOnboardedClient } from '@/lib/doormileHooks'; import AddressAutocomplete from '@/components/doormile/AddressAutocomplete'; -import { addressFromPlace, addressPayload, generateClientPassword, validateAddress } from '@/lib/clientOnboarding'; +import { + DELIVERY_CATEGORIES, + addressFromPlace, + addressPayload, + generateClientPassword, + reverseLogisticsAllowed, + validateAddress +} from '@/lib/clientOnboarding'; /** * Edit and Remove for one onboarded client login (a row of "Clients with a @@ -24,6 +31,10 @@ export function changedFields(row, form) { if (form.phone.replace(/\D/g, '') !== String(row.primarycontact || '').replace(/\D/g, '')) out.phone = form.phone.replace(/\D/g, ''); if (form.status !== row.status) out.status = form.status; if (Boolean(form.requiredeliveryotp) !== Boolean(row.requiredeliveryotp)) out.requiredeliveryotp = Boolean(form.requiredeliveryotp); + // Omitted when unchanged: the server leaves an absent category alone, which + // is what lets a client onboarded before this field existed be edited + // without being forced to pick one. + if (trim(form.deliverycategory) !== trim(row.deliverycategory)) out.deliverycategory = trim(form.deliverycategory); if (form.password) out.password = form.password; // The address goes as one `location` object, and only when any part of it // changed (a coordinate change alone counts: re-picking the same text moves @@ -69,6 +80,7 @@ function formFrom(row) { email: row?.loginemail || '', phone: String(row?.primarycontact || '').replace(/\D/g, ''), status: STATUSES.includes(row?.status) ? row.status : 'Active', + deliverycategory: row?.deliverycategory || '', requiredeliveryotp: Boolean(row?.requiredeliveryotp), password: '', address: row?.address || '', @@ -235,6 +247,31 @@ export function EditClientModal({ row, onClose }) { set('city')(e.target.value)} /> + + + diff --git a/src/pages/doormile/clients/ClientOnboarding.jsx b/src/pages/doormile/clients/ClientOnboarding.jsx index d90bc40..5b8c922 100644 --- a/src/pages/doormile/clients/ClientOnboarding.jsx +++ b/src/pages/doormile/clients/ClientOnboarding.jsx @@ -1,6 +1,9 @@ import React, { useMemo, useState } from 'react'; import { useNavigate } from 'react-router-dom'; -import { ArrowLeft, Building2, Copy, Eye, EyeOff, KeyRound, MapPin, Pencil, ShieldCheck, Trash2, UserPlus } from 'lucide-react'; +import { + ArrowLeft, Ban, Building2, Copy, Eye, EyeOff, KeyRound, MapPin, + Pencil, RotateCcw, ShieldCheck, Trash2, UserPlus +} from 'lucide-react'; import { Alert, Breadcrumb, BreadcrumbItem, BreadcrumbLink, BreadcrumbList, BreadcrumbPage, BreadcrumbSeparator, Button, DataTable, EmptyState, Field, Input, PageHeader, Stack, @@ -11,10 +14,7 @@ import { useAuth } from '@/lib/AuthContext'; import { useOnboardClient, useOnboardedClients, useOnboardingCities } from '@/lib/doormileHooks'; import AddressAutocomplete from '@/components/doormile/AddressAutocomplete'; import { EditClientModal, RemoveClientModal } from './ClientLoginDialogs'; -import { - addressFromPlace, addressPayload, canOnboardClients, generateClientPassword, hasMapLocation, - onboardingUnavailableMessage, validateOnboarding, -} from '@/lib/clientOnboarding'; +import { addressFromPlace, addressPayload, canOnboardClients, generateClientPassword, hasMapLocation, onboardingUnavailableMessage, validateOnboarding, DELIVERY_CATEGORIES, reverseLogisticsAllowed } from '@/lib/clientOnboarding'; /** * Client onboarding — register a new client and give them a console login. @@ -36,6 +36,7 @@ const EMPTY = { applocationid: '', password: '', confirm: '', + deliverycategory: '', requiredeliveryotp: false, // Main address, saved as the client's primary location. Coordinates come // only from picking a suggestion. @@ -54,6 +55,8 @@ const SERVER_FIELD = [ ['mobile', 'phone'], ['password', 'password'], ['operating city', 'applocationid'], + ['delivers', 'deliverycategory'], + ['delivery category', 'deliverycategory'], ['pincode', 'pincode'], ['address', 'address'], ['map location', 'address'], @@ -198,6 +201,7 @@ export default function ClientOnboarding() { phone: form.phone.replace(/\D/g, ''), password, applocationid: Number(form.applocationid), + deliverycategory: form.deliverycategory, requiredeliveryotp: form.requiredeliveryotp, ...addressPayload(form), }, @@ -350,7 +354,13 @@ export default function ClientOnboarding() { /> - + + + + +
+ + {/* + Reverse logistics follows from WHAT they ship, so it is stated + rather than asked — the operator answers the question they can + answer ("what do they deliver?") and sees the consequence. + + When it is off, say so rather than hiding the row silently: an + absent control reads as an oversight, and the operator needs to + know this client has no return path before they promise one. + + The backend enforces this independently (constants.ReverseLogisticsAllowed, + checked in InitiateConsignmentRTO). This is presentation only. + */} + {form.deliverycategory && ( +
+
+ {reverseLogisticsAllowed(form.deliverycategory) ? ( + + ) : ( + + )} +
+ + {reverseLogisticsAllowed(form.deliverycategory) + ? 'Reverse logistics available' + : 'No reverse logistics'} + + + {reverseLogisticsAllowed(form.deliverycategory) + ? ' — undelivered parcels can be returned to this client.' + : ' — a returned meal cannot be restocked or resold, so returns stay off for food clients.'} + +
+
+
+ )} +
diff --git a/tests/api/endpoints.test.js b/tests/api/endpoints.test.js index 8804542..a7bfbe2 100644 --- a/tests/api/endpoints.test.js +++ b/tests/api/endpoints.test.js @@ -288,9 +288,13 @@ describe('doormile endpoints', () => { expect(mockClient.post).toHaveBeenCalledWith('/admin/bookings/4021/assign-vehicle', { vehicleid: 5 }); }); + // ADMIN, not hub. /hub/bookings/batch-assign sits behind HubStaffAuth and + // 403s for every token this console issues, so this function could never + // succeed while it pointed there. /admin/bookings/batch-assign runs the + // same solver (controllers/batchAssignService.go) under admin auth. it('should batch-assign with a default cap of five stops per rider', async () => { await batchAssignBookings([1, 2, 3]); - expect(mockClient.post).toHaveBeenCalledWith('/hub/bookings/batch-assign', { + expect(mockClient.post).toHaveBeenCalledWith('/admin/bookings/batch-assign', { bookingids: [1, 2, 3], max_per_rider: 5 }); @@ -298,12 +302,19 @@ describe('doormile endpoints', () => { it('should honour an explicit per-rider cap', async () => { await batchAssignBookings([1, 2], 2); - expect(mockClient.post).toHaveBeenCalledWith('/hub/bookings/batch-assign', { + expect(mockClient.post).toHaveBeenCalledWith('/admin/bookings/batch-assign', { bookingids: [1, 2], max_per_rider: 2 }); }); + it('should not call the hub-only route, which 403s from this console', async () => { + await batchAssignBookings([1]); + expect(mockClient.post).not.toHaveBeenCalledWith( + '/hub/bookings/batch-assign', expect.anything() + ); + }); + it('should bulk-cancel through the dedicated route', async () => { await bulkCancelBookings([1, 2, 3]); expect(mockClient.post).toHaveBeenCalledWith('/admin/bookings/bulk-cancel', { bookingIds: [1, 2, 3] }); diff --git a/tests/integration/clientOnboarding.test.jsx b/tests/integration/clientOnboarding.test.jsx index 8546e13..b6fa7c9 100644 --- a/tests/integration/clientOnboarding.test.jsx +++ b/tests/integration/clientOnboarding.test.jsx @@ -97,6 +97,7 @@ const fillValidForm = async () => { fill('Mobile number', '98765 43210'); fill('Login email', ' Ops@Acme.Example '); fill('Operating city', '1'); + fill('What do they deliver', 'Clothing'); fireEvent.change(document.getElementById('onboard-password'), { target: { value: 's3cure-pass' } }); fill('Confirm password', 's3cure-pass'); fireEvent.click(screen.getAllByRole('button', { name: 'Pick Client address' })[0]); @@ -221,6 +222,7 @@ describe('Client onboarding page', () => { phone: '9876543210', password: 's3cure-pass', applocationid: 1, + deliverycategory: 'Clothing', requiredeliveryotp: false, address: '14 DB Road, RS Puram, Coimbatore', city: 'Coimbatore', @@ -263,6 +265,7 @@ describe('Client onboarding page', () => { fill('Mobile number', '9876543210'); fill('Login email', 'ops@acme.example'); fill('Operating city', '1'); + fill('What do they deliver', 'Clothing'); fireEvent.change(document.getElementById('onboard-password'), { target: { value: 's3cure-pass' } }); fill('Confirm password', 's3cure-pass'); // Typed, never picked: no map location. @@ -281,6 +284,41 @@ describe('Client onboarding page', () => { expect(await screen.findByText('5 Avinashi Road · 641004')).toBeInTheDocument(); }); + // The feature itself: what they ship decides whether returns apply. + it('offers reverse logistics for a clothing client', async () => { + renderPage(); + await fillValidForm(); + fill('What do they deliver', 'Clothing'); + expect(await screen.findByText(/Reverse logistics available/i)).toBeInTheDocument(); + expect(screen.getByText(/can be returned to this client/i)).toBeInTheDocument(); + }); + + // A returned meal is waste, not inventory — nothing to restock or resell. + it('withdraws reverse logistics for a food client, and says why', async () => { + renderPage(); + await fillValidForm(); + fill('What do they deliver', 'Food'); + expect(await screen.findByText(/No reverse logistics/i)).toBeInTheDocument(); + // Stated, not silently hidden: an absent row reads as an oversight, and the + // operator needs to know before promising a client a return path. + expect(screen.getByText(/cannot be restocked or resold/i)).toBeInTheDocument(); + }); + + it('says nothing about returns until a category is chosen', () => { + renderPage(); + expect(screen.queryByText(/Reverse logistics available/i)).not.toBeInTheDocument(); + expect(screen.queryByText(/No reverse logistics/i)).not.toBeInTheDocument(); + }); + + it('refuses to submit without a delivery category', async () => { + renderPage(); + await fillValidForm(); + fill('What do they deliver', ''); + fireEvent.click(screen.getByRole('button', { name: /Onboard client/ })); + expect(await screen.findByText(/choose what this client delivers/i)).toBeInTheDocument(); + expect(api.onboardClient).not.toHaveBeenCalled(); + }); + it('puts a server conflict under the field it is about', async () => { api.onboardClient.mockRejectedValue({ response: { status: 409, data: { success: false, message: 'this email already has a console login' } } }); renderPage(); @@ -359,7 +397,7 @@ describe('client onboarding helpers', () => { expect(pw).toMatch(/\d/); expect(pw).toMatch(/[a-z]/i); const errors = validateOnboarding({ - companyname: 'Acme', contactname: 'Priya', email: 'a@b.co', phone: '9876543210', applocationid: 1, password: pw, confirm: pw, + companyname: 'Acme', contactname: 'Priya', email: 'a@b.co', phone: '9876543210', applocationid: 1, deliverycategory: 'Clothing', password: pw, confirm: pw, address: '14 DB Road, RS Puram', pincode: '641002', latitude: 11.009, longitude: 76.95, }); expect(errors).toEqual({}); @@ -374,7 +412,7 @@ describe('client onboarding helpers', () => { }); it('mirrors the server rules', () => { - const base = { companyname: 'Acme', contactname: 'Priya', email: 'a@b.co', phone: '9876543210', applocationid: 1, password: 'longenough1', confirm: 'longenough1' }; + const base = { companyname: 'Acme', contactname: 'Priya', email: 'a@b.co', phone: '9876543210', applocationid: 1, deliverycategory: 'Clothing', password: 'longenough1', confirm: 'longenough1' }; expect(validateOnboarding({ ...base, phone: '5876543210' }).phone).toBeTruthy(); expect(validateOnboarding({ ...base, email: 'Ops ' }).email).toBeTruthy(); expect(validateOnboarding({ ...base, password: 'a@b.co', confirm: 'a@b.co' }).password).toBeTruthy(); diff --git a/tests/lib/agentActions.test.js b/tests/lib/agentActions.test.js index 81f0615..385adea 100644 --- a/tests/lib/agentActions.test.js +++ b/tests/lib/agentActions.test.js @@ -38,16 +38,31 @@ beforeEach(() => { describe('which proposals can be executed', () => { it('only verbs with a real endpoint behind them', () => { - expect(executableTools().sort()).toEqual(['notifyRider']); + // alert_low_battery_rider joined notifyRider: it targets the same + // POST /admin/milers/:id/notify, so it was never a missing capability — + // just a verb nothing had been wired to. + expect(executableTools().sort()).toEqual([ + 'alert_low_battery_rider', 'assignMiler', 'notifyRider', 'trigger_auto_dispatch' + ]); }); it('canExecuteProposal is false for the skills tools with no endpoint', () => { - ['trigger_auto_dispatch', 'enforce_cash_handoff', 'enforce_otp_verification', 'alert_low_battery_rider'] + // Still review-only. Each needs an endpoint that does not exist yet — + // seed.go marks all three Target: "none yet". + ['enforce_cash_handoff', 'enforce_otp_verification', 'dispatch_hub_idle_parcels'] .forEach((tool) => { expect(canExecuteProposal(finding(tool, [row()]))).toBe(false); }); }); + it('alert_low_battery_rider notifies the riders on the finding', async () => { + const result = await executeProposal(finding('alert_low_battery_rider', [row()])); + expect(result.ok).toBe(true); + // Keyed on milerprofileid, not userid — the two are different small + // integers on the same record and the wrong one fails silently. + expect(api.notifyRider).toHaveBeenCalledWith(99, expect.any(String), expect.stringMatching(/charge/i)); + }); + // Resolving quietly would let a caller mistake "nothing happened" for success. it('throws rather than silently doing nothing', async () => { await expect(executeProposal(finding('enforce_cash_handoff', [row()]))).rejects.toThrow(/review-only/i); @@ -130,18 +145,92 @@ describe('notifyRider', () => { }); describe('assignMiler', () => { - // Deliberately review-only in the console. The branch executed it through - // POST /hub/bookings/batch-assign, which is behind HubStaffAuth (roleid 6 - // only) — every admin/manager/executive click would 403. Until an admin - // batch-assign route exists, the card must say "Review only", not fail. - it('is not executable, so its card renders "Review only"', () => { - expect(canExecuteProposal(finding('assignMiler', [row({ bookingid: 1 })]))).toBe(false); + // This was review-only because POST /hub/bookings/batch-assign is behind + // HubStaffAuth (roleid 6 only) and 403s for every token this console issues. + // It is executable now that POST /admin/bookings/batch-assign runs the same + // solver under admin auth. + it('is executable', () => { + expect(canExecuteProposal(finding('assignMiler', [row({ bookingid: 1 })]))).toBe(true); }); - it('throws instead of pretending, and never calls the hub-only endpoint', async () => { - await expect(executeProposal(finding('assignMiler', [row()]))).rejects.toThrow(/review-only/); + it('sends the findings distinct booking ids, deduplicated', async () => { + api.batchAssignBookings.mockResolvedValue({ assigned: 2, skipped: 0, riderssequenced: 1, results: [] }); + await executeProposal(finding('assignMiler', [ + row({ bookingid: 11 }), row({ bookingid: 12 }), row({ bookingid: 11 }) + ])); + expect(api.batchAssignBookings).toHaveBeenCalledWith([11, 12]); + }); + + // The rule the whole module exists for: never report an action that did not + // happen. Four of nine landing is four of nine. + it('reports a partial assignment as partial, naming the reasons', async () => { + api.batchAssignBookings.mockResolvedValue({ + assigned: 1, skipped: 1, riderssequenced: 1, + results: [ + { bookingid: 11, bookingno: 'DM-11', assigned: true, mileruserid: 7 }, + { bookingid: 12, bookingno: 'DM-12', assigned: false, reason: 'no available rider under capacity' } + ] + }); + const res = await executeProposal(finding('assignMiler', [row({ bookingid: 11 }), row({ bookingid: 12 })])); + expect(res.ok).toBe(true); + // Declared so reportActed records "partial" rather than "ok". Caught by + // driving the real page: the message said partial, the telemetry said ok. + expect(res.partial).toBe(true); + expect(res.message).toMatch(/Assigned 1 order/); + expect(res.message).toMatch(/DM-12 \(no available rider under capacity\)/); + }); + + // The mocks above return a FLAT object, but batchAssignBookings returns + // `response.data` — and utils.OK wraps everything as {success, data}. So + // production hands the executor {data: {assigned, skipped, ...}}. Testing + // only the flat shape would leave the real one unproven. + it('reads the real utils.OK envelope, not just a flat object', async () => { + api.batchAssignBookings.mockResolvedValue({ + success: true, + data: { + assigned: 1, skipped: 1, riderssequenced: 1, + results: [ + { bookingid: 11, bookingno: 'DM-11', assigned: true, mileruserid: 7 }, + { bookingid: 12, bookingno: 'DM-12', assigned: false, reason: 'no available rider under capacity' } + ] + } + }); + const res = await executeProposal(finding('assignMiler', [row({ bookingid: 11 }), row({ bookingid: 12 })])); + expect(res.ok).toBe(true); + expect(res.message).toMatch(/Assigned 1 order/); + expect(res.message).toMatch(/DM-12/); + }); + + it('is not ok when nothing was assigned', async () => { + api.batchAssignBookings.mockResolvedValue({ + assigned: 0, skipped: 1, riderssequenced: 0, + results: [{ bookingid: 12, bookingno: 'DM-12', assigned: false, reason: 'no available rider under capacity' }] + }); + const res = await executeProposal(finding('assignMiler', [row({ bookingid: 12 })])); + expect(res.ok).toBe(false); + expect(res.sourceCalls[0].status).toBe('error'); + }); + + // Stop sequencing needs ROUTE_OPTIMIZER_URL set in the cluster. Saying + // nothing when it is zero leaves the operator assuming routes were ordered. + it('says so when stops were not sequenced', async () => { + api.batchAssignBookings.mockResolvedValue({ assigned: 2, skipped: 0, riderssequenced: 0, results: [] }); + const res = await executeProposal(finding('assignMiler', [row({ bookingid: 11 }), row({ bookingid: 12 })])); + expect(res.message).toMatch(/not sequenced/i); + }); + + it('refuses findings with no booking rather than calling the endpoint', async () => { + const res = await executeProposal(finding('assignMiler', [row({ bookingid: undefined })])); + expect(res.ok).toBe(false); expect(api.batchAssignBookings).not.toHaveBeenCalled(); }); + + it('reports a thrown error instead of claiming success', async () => { + api.batchAssignBookings.mockRejectedValue(new Error('403 forbidden')); + const res = await executeProposal(finding('assignMiler', [row({ bookingid: 11 })])); + expect(res.ok).toBe(false); + expect(res.message).toMatch(/403/); + }); }); // The gap the tests above have: they mock buildMilerLookup, so the id diff --git a/tests/lib/findingReport.test.js b/tests/lib/findingReport.test.js new file mode 100644 index 0000000..f4ca3e2 --- /dev/null +++ b/tests/lib/findingReport.test.js @@ -0,0 +1,157 @@ +/** + * Finding persistence. + * + * The fingerprint is the part worth testing hardest. It decides whether a + * finding seen on two consecutive polls is the SAME finding — and if it is + * wrong, the "how long has this been open" measurement, which is the single + * most useful thing the table records, is destroyed. + */ +jest.mock('@/api/doormile', () => ({ + reportFindings: jest.fn().mockResolvedValue({}), + reportFindingActed: jest.fn().mockResolvedValue({}) +})); + +const api = require('@/api/doormile'); +const { fingerprintOf, reportScan, reportActed, __resetForTests } = + require('@/lib/assistant/agent/findingReport'); + +const finding = (skillId, bookingids, extra = {}) => ({ + skillId, + severity: 'warning', + title: 'Something needs attention', + proposal: { tool: 'notifyRider', scope: bookingids.map((bookingid) => ({ bookingid })) }, + ...extra +}); + +beforeEach(() => { + jest.clearAllMocks(); + __resetForTests(); + // The module reads tenantid from localStorage. + localStorage.setItem('tenantid', '7'); +}); + +describe('fingerprintOf', () => { + it('is stable across scans regardless of scope order', () => { + // Scope order is not guaranteed between scans; an unsorted fingerprint + // would make the same problem look new every poll. + expect(fingerprintOf(finding('SlaGuardian', [3, 1, 2]))) + .toBe(fingerprintOf(finding('SlaGuardian', [1, 2, 3]))); + }); + + it('ignores severity, so an escalating finding stays the same finding', () => { + // An ageing SLA breach climbs warning -> critical while the underlying + // problem is unchanged. Including severity would reset firstseenat and + // report a six-hour-old problem as brand new. + const a = fingerprintOf(finding('SlaGuardian', [1], { severity: 'warning' })); + const b = fingerprintOf(finding('SlaGuardian', [1], { severity: 'critical' })); + expect(a).toBe(b); + }); + + it('distinguishes two proposals from ONE skill over the same bookings', () => { + // SlaGuardian raises both "notify the riders" and "assign these orders" + // for the same parcels. Keyed on skill + scope alone these collide, and + // since the fingerprint is the upsert key one would silently overwrite the + // other. Found by running the real page, not by these tests. + const notify = finding('SlaGuardian', [1, 2, 3]); + const assign = { ...finding('SlaGuardian', [1, 2, 3]), proposal: { tool: 'assignMiler', scope: [1, 2, 3].map((bookingid) => ({ bookingid })) } }; + expect(fingerprintOf(notify)).not.toBe(fingerprintOf(assign)); + }); + + it('distinguishes different skills on the same booking', () => { + expect(fingerprintOf(finding('SlaGuardian', [1]))) + .not.toBe(fingerprintOf(finding('CashExposure', [1]))); + }); + + it('distinguishes different scopes', () => { + expect(fingerprintOf(finding('SlaGuardian', [1]))) + .not.toBe(fingerprintOf(finding('SlaGuardian', [1, 2]))); + }); + + it('survives a missing scope rather than throwing', () => { + expect(fingerprintOf({ skillId: 'SlaGuardian' })).toBe('SlaGuardian:none:'); + expect(fingerprintOf({})).toBe('unknown:none:'); + }); +}); + +describe('reportScan', () => { + it('sends ids only, never personal data', async () => { + await reportScan([finding('SlaGuardian', [11])]); + const payload = api.reportFindings.mock.calls[0][0]; + const serialised = JSON.stringify(payload); + expect(payload.findings[0].bookingids).toEqual([11]); + // The findings table measures the skills; it is not a second copy of the + // bookings. + expect(serialised).not.toMatch(/phone|address|recipient|ridername/i); + }); + + it('reports what cleared since the previous scan', async () => { + await reportScan([finding('SlaGuardian', [1]), finding('CashExposure', [2])]); + api.reportFindings.mockClear(); + + await reportScan([finding('SlaGuardian', [1])]); + const payload = api.reportFindings.mock.calls[0][0]; + expect(payload.cleared).toEqual(['CashExposure:notifyRider:2']); + }); + + it('skips the round trip when nothing is open and nothing cleared', async () => { + await reportScan([]); + expect(api.reportFindings).not.toHaveBeenCalled(); + }); + + it('still reports when everything clears at once', async () => { + await reportScan([finding('SlaGuardian', [1])]); + api.reportFindings.mockClear(); + await reportScan([]); + expect(api.reportFindings).toHaveBeenCalledWith({ + findings: [], cleared: ['SlaGuardian:notifyRider:1'] + }); + }); + + it('never throws when the backend refuses', async () => { + api.reportFindings.mockRejectedValueOnce(new Error('403')); + // A dispatcher looking at a stalled parcel does not care that telemetry is + // down, and must not be shown an error about it. + await expect(reportScan([finding('SlaGuardian', [1])])).resolves.toBeUndefined(); + }); + + it('retries the clear set after a failure instead of losing it', async () => { + await reportScan([finding('SlaGuardian', [1])]); + api.reportFindings.mockClear(); + + // The scan where it clears fails... + api.reportFindings.mockRejectedValueOnce(new Error('503')); + await reportScan([]); + + // ...so the next scan must still report it cleared. Advancing `previous` + // on a failed post would drop the clear permanently and leave the finding + // open forever. + await reportScan([]); + const last = api.reportFindings.mock.calls.at(-1)[0]; + expect(last.cleared).toEqual(['SlaGuardian:notifyRider:1']); + }); +}); + +describe('reportActed', () => { + it('records a clean success as ok', async () => { + await reportActed(finding('SlaGuardian', [1]), { ok: true }); + expect(api.reportFindingActed).toHaveBeenCalledWith('SlaGuardian:notifyRider:1', 'ok'); + }); + + it('records a partial success as partial, not ok', async () => { + // Six riders notified out of eight is not a success. Flattening it would + // lose exactly the distinction actions.js preserves. + await reportActed(finding('SlaGuardian', [1, 2]), { ok: true, unreachable: ['DM-2 (no device)'] }); + expect(api.reportFindingActed).toHaveBeenCalledWith('SlaGuardian:notifyRider:1,2', 'partial'); + }); + + it('records a failure', async () => { + await reportActed(finding('SlaGuardian', [1]), { ok: false }); + expect(api.reportFindingActed).toHaveBeenCalledWith('SlaGuardian:notifyRider:1', 'failed'); + }); + + it('never throws when the backend refuses', async () => { + api.reportFindingActed.mockRejectedValueOnce(new Error('500')); + // The action itself already happened and was reported to the operator. + await expect(reportActed(finding('SlaGuardian', [1]), { ok: true })).resolves.toBeUndefined(); + }); +});