diff --git a/apps/ai_agent/docs/concepts-prompts-and-models.md b/apps/ai_agent/docs/concepts-prompts-and-models.md new file mode 100644 index 0000000..00397f4 --- /dev/null +++ b/apps/ai_agent/docs/concepts-prompts-and-models.md @@ -0,0 +1,311 @@ +# Prompts and model configuration + +The system prompts behind each LLM-backed endpoint, the model used per endpoint, how raw model output is parsed back into typed responses, and what happens when the model returns something malformed. + +Everything here reflects [apps/ai_agent/main.py](../main.py) as implemented. Prompts are reproduced **verbatim** — if you change one in code, change it here in the same commit, because a drifted prompt doc is worse than no prompt doc. + +## Models at a glance + +| Endpoint | Model | Call type | JSON mode | Timeout | +| ---------------------- | ------------------------ | --------------- | --------- | ------- | +| `POST /chat` | `gpt-4o-mini` | chat completion | no | 30s | +| `POST /transfers/analyse` | `gpt-4o-mini` | chat completion | **yes** | 10s | +| `POST /proposals/summarise` | `gpt-4o-mini` | chat completion | **yes** | 10s | +| `POST /index/message` | `text-embedding-3-small` | embedding | n/a | none | +| `GET /search` | `text-embedding-3-small` | embedding | n/a | none | + +Model identifiers are **hardcoded string literals at each call site**. There is no configuration object, no environment variable, and no central constant. Changing a model means editing `main.py`. + +`GET /health` involves no model at all. + +## The prompts + +### `POST /chat` — system prompt + +This is the only true *system* prompt in the service. It is a module-level constant, applied to every `/chat` request: + +```python +_SYSTEM_PROMPT = ( + "You are an AI assistant for Clicked, a decentralised messaging and payment " + "platform built on the Stellar blockchain. Clicked lets users send token " + "payments inside chat conversations, manage group treasuries, and participate " + "in DAO-style governance. Help users with questions about transactions, wallet " + "management, group finances, and platform features." +) +``` + +Assembled as: + +```python +response = client.chat.completions.create( + model="gpt-4o-mini", + messages=[ + {"role": "system", "content": _SYSTEM_PROMPT}, + {"role": "user", "content": request.message}, + ], + timeout=30, +) +``` + +**Behaviour it is designed to produce.** The prompt does one job: domain grounding. It tells the model what Clicked is (decentralised messaging + payments), what chain it runs on (Stellar), what users can do (in-chat payments, group treasuries, DAO governance), and which question areas are in scope (transactions, wallet management, group finances, platform features). Without it the model would answer Stellar questions generically and would have no idea what "group treasury" means in this product. + +Three properties of this prompt matter operationally: + +- **It does not constrain output format.** `/chat` returns free-form prose; there is no JSON mode and no schema. The reply is passed through unmodified. +- **It does not constrain output length.** There is no `max_tokens`, so response length — and cost — is bounded only by the model. +- **It carries no conversation history.** Each request sends exactly two messages: system and the current user turn. `ChatRequest.conversation_id` is accepted and validated by Pydantic but **is never sent to the model and never used to load prior turns**. `/chat` is stateless and single-turn despite the field's name implying otherwise. + +`test_system_prompt_contains_context` in [tests/test_chat.py](../tests/test_chat.py) pins the presence of the "Clicked", "Stellar", and messaging/payment grounding, so gutting the prompt fails CI. + +### `POST /transfers/analyse` — user prompt + +No system message. The whole instruction is a single user-role message, built per request: + +```python + prompt = ( + "Analyse this Stellar transfer for fraud risk.\n" + f"Amount: {request.amount} XLM\n" + f"Sender: {request.sender}\n" + f"Recipient: {request.recipient}\n" + f"Memo: {request.memo}\n\n" + "Reply with JSON only using keys: flagged (bool), reason (string under 100 chars or null), " + "confidence (float 0-1). Flag if suspicious patterns are detected." + ) +``` + +Sent as: + +```python +response = client.chat.completions.create( + model="gpt-4o-mini", + messages=[{"role": "user", "content": prompt}], + response_format={"type": "json_object"}, + timeout=10, +) +``` + +**Behaviour it is designed to produce.** A three-key JSON verdict — `flagged`, `reason`, `confidence` — matching the `TransferAnalyseResponse` shape field for field. The `reason` length hint keeps the string renderable in a UI, and the `confidence` range makes the score comparable across calls. `response_format={"type": "json_object"}` puts the model in JSON mode so output is guaranteed to be syntactically valid JSON — though *not* guaranteed to contain the keys asked for, which is the whole reason the parsing layer below exists. + +**This prompt is only reached for transfers at or below the threshold.** Amounts strictly greater than `_HIGH_VALUE_THRESHOLD` (`10_000.0`) return a rule-based verdict without any model call: + +```python +if request.amount > _HIGH_VALUE_THRESHOLD: + return TransferAnalyseResponse( + flagged=True, + reason=f"Amount {request.amount} XLM exceeds {_HIGH_VALUE_THRESHOLD} XLM threshold", + confidence=0.99, + ) +``` + +The high-value flag is deliberately not delegated to the model — it is a security property that must not depend on model behaviour. See [concepts-transfer-risk-analysis.md](concepts-transfer-risk-analysis.md) for the rationale. + +### `POST /proposals/summarise` — user prompt + +Again a single user-role message, no system message: + +```python + prompt = ( + "Summarise this Clicked governance proposal for a frontend reader and " + "rate its risk level.\n" + f"Title: {request.title}\n" + f"Description: {request.description}\n" + f"Amount: {request.amount} XLM\n\n" + "Reply with JSON only using keys: summary (a plain-English summary of " + 'exactly 2 sentences), risk (one of "low", "medium", "high"). ' + 'Use "high" for large amounts, unclear intent, or obvious red flags; ' + '"low" for small, well-scoped, low-impact proposals; otherwise "medium".' + ) +``` + +Sent as: + +```python +response = client.chat.completions.create( + model="gpt-4o-mini", + messages=[{"role": "user", "content": prompt}], + response_format={"type": "json_object"}, + timeout=10, +) +``` + +**Behaviour it is designed to produce.** Two outputs in one call. The `summary` is constrained to *exactly two sentences* and to plain English, because it renders in a fixed-size UI slot and is read by voters who will not read the full proposal. The `risk` field is constrained to a closed three-value set, and — unlike the transfer prompt — the prompt supplies an explicit rubric for choosing between them: `high` for large amounts, unclear intent, or obvious red flags; `low` for small, well-scoped, low-impact proposals; `medium` otherwise. `medium` is the stated default, which matters because it is also the code's fallback (see below), so a malformed response degrades toward the same value the rubric already treats as neutral. + +### Embedding calls — `/index/message` and `/search` + +No prompt. Both endpoints embed raw text directly with `text-embedding-3-small`: + +```python +# /index/message +res = openai_client.embeddings.create(input=request.content, model="text-embedding-3-small") +vector = res.data[0].embedding + +# /search +res = openai_client.embeddings.create(input=q, model="text-embedding-3-small") +vector = res.data[0].embedding +``` + +**Both sides must use the same model.** Vectors from different embedding models are not comparable, so a query embedded with one model against a corpus indexed with another returns semantically meaningless results — with no error. This is the most dangerous coupling in the service; see [Changing a model safely](#changing-a-model-safely). + +## Output parsing and validation + +Each endpoint hands model output back through a different amount of validation. Ordered from least to most defensive: + +### `/chat` — no parsing + +```python +return ChatResponse(reply=response.choices[0].message.content) +``` + +The content string is passed straight into `ChatResponse`. There is no JSON parsing and no content validation. + +One sharp edge: the OpenAI API can return `None` for `message.content` (for example on a content-filter stop). `ChatResponse.reply` is typed `str`, so a `None` raises a Pydantic `ValidationError` inside the handler, which surfaces to the caller as an unhandled `500`. There is no explicit guard for this case. + +### `/transfers/analyse` — parse with per-field defaults + +This is the worked example the parsing strategy is best understood through: + +```python +result = json.loads(response.choices[0].message.content) +return TransferAnalyseResponse( + flagged=bool(result.get("flagged", False)), + reason=result.get("reason"), + confidence=float(result.get("confidence", 0.0)), +) +``` + +Every field is read with `.get()` and a default, so a **partial** JSON object still produces a valid typed response: + +| Model returns | Parsed result | Rationale | +| ------------- | ------------- | --------- | +| `{"flagged": true, "reason": "Suspicious memo", "confidence": 0.9}` | `flagged=True, reason="Suspicious memo", confidence=0.9` | Complete response, passed through. | +| `{"flagged": false, "reason": null}` — **`confidence` missing** | `flagged=False, reason=None, **confidence=0.0**` | `.get("confidence", 0.0)` supplies `0.0`. | +| `{"reason": null, "confidence": 0.5}` — **`flagged` missing** | **`flagged=False`**, `reason=None, confidence=0.5` | `.get("flagged", False)` supplies `False`. | +| `{}` | `flagged=False, reason=None, confidence=0.0` | Both defaults apply. | + +**The two defaults fail in opposite directions, and the asymmetry is intentional.** + +A missing `flagged` defaults to `False` — *do not flag*. A missing `confidence` defaults to `0.0` — *no confidence at all*. So a model that omits `flagged` produces a "not flagged" verdict, and a model that omits `confidence` produces a verdict the caller can see is worthless. A consumer that reads `confidence` before acting on `flagged` will correctly distrust both malformed cases. A consumer that reads `flagged` alone will silently treat a malformed response as a clean transfer. + +**This is the operationally important consequence: on this endpoint, model failure looks like "transfer is fine".** Callers making a security decision must gate on `confidence`, not on `flagged` alone. Both default paths are pinned by `test_llm_path_missing_confidence_defaults_to_zero` and `test_llm_path_missing_flagged_defaults_to_false` in [tests/test_transfers.py](../tests/test_transfers.py). + +Note also that `confidence` is **not range-checked**. The prompt asks for `0-1`, but a model returning `7.5` produces `confidence=7.5`; `float()` only enforces the type. Similarly `reason` has no length check despite the prompt's "under 100 chars". + +If the model returns syntactically invalid JSON, `json.loads` raises and the request fails as an unhandled `500`. JSON mode makes this unlikely but not impossible (a response truncated by the token limit is still invalid JSON). There is no `try/except` around the parse. + +### `/proposals/summarise` — parse, reject, then clamp + +The most defensive of the three, and the only one that deliberately returns a `502`: + +```python +result = json.loads(response.choices[0].message.content) + +summary = (result.get("summary") or "").strip() +if not summary: + raise HTTPException(status_code=502, detail="LLM did not return a summary") + +risk = str(result.get("risk", "")).strip().lower() +if risk not in ("low", "medium", "high"): + # Defensive fallback: never return an invalid risk level to the caller. + risk = "medium" + +# Pydantic re-validates via response_model before the response is sent. +return ProposalSummariseResponse( + summary=summary, risk=cast(Literal["low", "medium", "high"], risk) +) +``` + +The two fields are handled by opposite strategies, because their failure modes differ in kind: + +- **`summary` is mandatory — a missing one is an error.** Absent, `null`, empty, or whitespace-only all collapse to `""` via `(... or "").strip()` and raise `502 "LLM did not return a summary"`. There is no sensible fabricated summary, so the endpoint fails loudly rather than returning an empty string that would render as a blank card. +- **`risk` is clamped — an invalid one degrades.** The value is normalised (`str()`, `.strip()`, `.lower()`, so `"HIGH"` and `" high "` both survive) and checked against the closed set. Anything else — a missing key, `"critical"`, `"unknown"`, `null` — becomes `"medium"`. Returning an out-of-set risk would break `RiskLevel` typing for every downstream consumer, so the code guarantees a valid value. + +`"medium"` is a reasonable clamp target precisely because it is also the prompt's stated default for the unremarkable case. But note the ambiguity it creates: **a `risk` of `"medium"` may mean the model judged the proposal middling, or that it returned garbage.** The two are indistinguishable to the caller, and no signal is logged. Do not treat `"medium"` as a confident assessment. + +The `cast()` is a static-typing assertion only — it performs no runtime check. The real runtime guarantee comes from `response_model=ProposalSummariseResponse` on the route decorator, which re-validates the outgoing payload against the `RiskLevel` literal before it is serialised. + +### Parsing strategy summary + +| Endpoint | Malformed field | Behaviour | Caller sees | +| -------- | --------------- | --------- | ----------- | +| `/chat` | content is `None` | Pydantic `ValidationError`, unhandled | `500` | +| `/transfers/analyse` | invalid JSON | `json.loads` raises, unhandled | `500` | +| `/transfers/analyse` | `flagged` missing | defaults to `False` | `200`, unflagged | +| `/transfers/analyse` | `confidence` missing | defaults to `0.0` | `200`, zero confidence | +| `/transfers/analyse` | `confidence` out of range | passed through unchecked | `200`, e.g. `7.5` | +| `/proposals/summarise` | invalid JSON | `json.loads` raises, unhandled | `500` | +| `/proposals/summarise` | `summary` missing/empty | explicit rejection | `502` | +| `/proposals/summarise` | `risk` missing/invalid | clamped to `"medium"` | `200` | + +The typed response shapes themselves are documented in [contracts-pydantic-models.md](contracts-pydantic-models.md). + +## Changing a model safely + +Model IDs are string literals at each call site, so a change is a code edit and a deploy. The safe procedure differs sharply between the two model families. + +### Changing a chat model + +Applies to `/chat`, `/transfers/analyse`, and `/proposals/summarise`. + +1. **Confirm JSON mode support.** `/transfers/analyse` and `/proposals/summarise` pass `response_format={"type": "json_object"}`. A model that does not support it will error on every request. `/chat` does not use JSON mode and is unconstrained here. +2. **Change one endpoint at a time.** The three call sites are independent — there is no shared constant — so a model change is naturally scoped to one endpoint. Keep it that way; do not sweep all three in one edit. +3. **Re-test the parse layer, not just the happy path.** A new model may phrase `risk` differently (triggering the `"medium"` clamp more often) or omit `confidence` more frequently (silently producing unflagged verdicts). The defaults will hide this — no error, no log, just quietly degraded output. Diff real outputs before and after. +4. **Re-check the timeouts.** `/transfers/analyse` and `/proposals/summarise` allow only 10s. A slower or reasoning-style model can exceed that and turn a working endpoint into a timeout. +5. **Re-check cost.** There is no `max_tokens` on any call, so a more expensive model multiplies cost directly with no ceiling. See [operations.md](operations.md#cost-controls-on-the-llm-path). +6. **Update `test_correct_model_used`** in [tests/test_chat.py](../tests/test_chat.py), which asserts the `/chat` model literal and will fail on any change. + +Changing a chat model is reversible: revert the literal and redeploy. + +### Changing the embedding model + +**This is not a routine change and is not reversible by redeploy alone.** + +`text-embedding-3-small` appears at two call sites — the write path in `/index/message` and the read path in `/search`. Vectors produced by different embedding models occupy different spaces and are not comparable. Changing the model on only one side produces a search that returns confident nonsense with **no error and no warning**. Changing it on both sides leaves every previously indexed message in the old space, which is the same failure for all existing data. + +A correct migration requires: + +1. Change the literal at **both** call sites in the same commit. +2. Re-embed and re-index the entire existing `Message` corpus with the new model. +3. Cut over reads only once the re-index is complete — ideally via a new collection and an atomic switch, since `/search` against a half-migrated corpus silently mixes both spaces. + +Also confirm the new model's dimensionality is compatible with the `Message` collection configuration before starting — see [contracts-weaviate-schema.md](contracts-weaviate-schema.md). + +## Privacy boundary + +**What leaves the system.** Every LLM-backed endpoint sends its payload to OpenAI, the sole external model provider. Specifically: + +| Endpoint | Sent to OpenAI | +| -------- | -------------- | +| `POST /chat` | The `_SYSTEM_PROMPT` and the full `message` string, verbatim | +| `POST /transfers/analyse` | `amount`, `sender`, `recipient`, `memo` — interpolated into the prompt (LLM path only) | +| `POST /proposals/summarise` | `title`, `description`, `amount` — interpolated into the prompt | +| `POST /index/message` | The full `content` string of the message being indexed | +| `GET /search` | The full `q` query string | + +Two of these deserve emphasis. **`/transfers/analyse` transmits both Stellar addresses and the transfer memo** — the memo is application-controlled and may carry a chat message reference or user-supplied text. **`/index/message` transmits the complete plaintext body of every indexed message**, which makes it the single largest flow of user content to a third party in the service. + +**What never leaves the system.** + +- **`OPENAI_API_KEY`** is used to authenticate to OpenAI and is never placed in a prompt. +- **`ChatRequest.conversation_id`** is accepted and validated but never sent to the model — `/chat` builds its message list from `request.message` alone. +- **`IndexMessageRequest.messageId`, `conversationId`, and `senderId`** are never sent to OpenAI. Only `content` is embedded. The identifiers are written to Weaviate as properties alongside the vector, so **message content reaches OpenAI while message metadata stays local**. +- **The `conversationId` filter on `/search`** is applied inside Weaviate via `Filter.by_property("conversationId").equal(conversationId)`, not by the model. Conversation scoping is enforced locally. +- **The stored vectors and all Weaviate contents.** Weaviate is a local dependency (`connect_to_local()`); the corpus is never shipped to the model provider. Only the text being embedded transits, one call at a time. +- **Everything on the `/transfers/analyse` rule-based path.** Transfers above `10_000.0` XLM are resolved entirely in-process with no network call, so their addresses and memos never reach OpenAI at all. +- **`GET /health`** involves no external call whatsoever. + +**Boundary properties to be aware of.** + +- **The service performs no redaction, masking, or PII stripping.** Whatever the caller supplies is what the provider receives. +- **Message content sent to OpenAI is plaintext at that point.** Whatever end-to-end encryption applies in the wider platform (see [apps/web/docs/concepts-e2ee-architecture.md](../../web/docs/concepts-e2ee-architecture.md)) does not extend to `/index/message`: the endpoint receives and forwards cleartext. Indexing a message is a deliberate decision to expose its content to the model provider. +- **Retention is governed by the OpenAI account's data policy**, not by this service. The service keeps no record of what it sent — no prompt logging, no request archive. +- **There is no authentication on any endpoint.** Any party able to reach the service can cause data to be sent to OpenAI under your account. Network isolation is the controlling mitigation — see [operations.md](operations.md#known-operational-risks). + +## Related documents + +- [Operations guide](operations.md) — running the service, health checks, scaling, cost controls +- [Testing guide](testing.md) — testing prompt and parsing behaviour without a real model +- [Pydantic models](contracts-pydantic-models.md) — the typed request/response shapes +- [Transfer risk analysis](concepts-transfer-risk-analysis.md) — why the high-value rule bypasses the model +- [RAG search architecture](concepts-rag-search-architecture.md) — how embeddings are indexed and retrieved +- [Weaviate schema](contracts-weaviate-schema.md) — the `Message` collection and vector configuration +- [Chat API](api-chat.md) · [Transfers analyse API](api-transfers-analyse.md) · [Proposals summarise API](api-proposals-summarise.md) · [Index and search API](api-index-search.md) diff --git a/apps/ai_agent/docs/operations.md b/apps/ai_agent/docs/operations.md new file mode 100644 index 0000000..402e653 --- /dev/null +++ b/apps/ai_agent/docs/operations.md @@ -0,0 +1,284 @@ +# AI agent operations guide + +How to run the `ai_agent` service in production: the ASGI/uvicorn setup, what `/health` does and does not tell an orchestrator, the Weaviate dependency, scaling characteristics, and the state of cost controls on the LLM path. + +This document describes the service as implemented in [apps/ai_agent/main.py](../main.py). Where the current implementation lacks an operational control, this document says so explicitly rather than describing an intended design — see [Known operational risks](#known-operational-risks). + +## Overview + +`ai_agent` is a single-module FastAPI application. It exposes five endpoints: + +| Method | Path | External dependencies | +| ------ | ---------------------- | ----------------------------- | +| `GET` | `/health` | none | +| `POST` | `/chat` | OpenAI | +| `POST` | `/transfers/analyse` | OpenAI (conditionally — see below) | +| `POST` | `/proposals/summarise` | OpenAI | +| `POST` | `/index/message` | OpenAI **and** Weaviate | +| `GET` | `/search` | OpenAI **and** Weaviate | + +The application object is `app`, defined at module scope in `main.py`. It holds no background tasks, no scheduler, no connection pool, and no in-process cache. Every request is fully independent, which is what makes the scaling story simple (see [Scaling characteristics](#scaling-characteristics)). + +## Running the service + +### The production start command + +`main.py` ends with a `__main__` guard: + +```python +if __name__ == "__main__": + uvicorn.run(app, host="0.0.0.0", port=8000) +``` + +Running `python main.py` therefore starts a **single-process, single-worker** uvicorn server on port 8000. This is the development path. Do not use it in production: it gives you one worker, no process supervision, and no way to set worker counts or timeouts without editing source. + +For production, invoke uvicorn directly against the ASGI app path so that all server configuration lives in the command rather than in code: + +```bash +uvicorn main:app \ + --host 0.0.0.0 \ + --port 8000 \ + --workers 4 \ + --timeout-keep-alive 65 \ + --no-server-header +``` + +`main:app` resolves to the `app` object in `main.py`, so the process must start with `apps/ai_agent` as the working directory (or that directory on `PYTHONPATH`). This mirrors the test configuration, which sets `pythonpath = ["."]` in `[tool.pytest.ini_options]` in [pyproject.toml](../pyproject.toml). + +### Worker configuration + +The endpoints are defined with `def`, not `async def`. FastAPI runs synchronous path operation functions in a **thread pool** rather than on the event loop, so a single worker can serve multiple concurrent requests despite the blocking OpenAI and Weaviate calls. The default thread pool is bounded (40 threads in current AnyIO defaults), which sets the practical per-worker concurrency ceiling. + +Sizing guidance: + +- **Workers**: start at `2 × CPU cores`. The service is I/O-bound, not CPU-bound — nearly all wall-clock time is spent waiting on OpenAI or Weaviate — so worker count is governed by memory and by upstream rate limits, not by core count. +- **Memory**: each worker is a full Python process that imports `fastapi`, `openai`, and `weaviate-client`. Budget conservatively and measure; do not assume workers are cheap. +- **Keep-alive**: set `--timeout-keep-alive` above your load balancer's idle timeout so the balancer, not the server, closes idle connections. + +Because there is no shared state between requests, worker count can be changed freely without correctness consequences. + +### Required environment + +| Variable | Required by | Behaviour when absent | +| ---------------- | ------------------------------------------------------ | -------------------------------------------- | +| `OPENAI_API_KEY` | `/chat`, `/transfers/analyse`, `/proposals/summarise` | Those endpoints return `500`. `/health` still returns `200`. | +| `OPENAI_API_KEY` | `/index/message`, `/search` | Those endpoints return **`503`**, not `500` — see the note below. | + +**The two Weaviate endpoints report a missing API key as `503`.** `_openai_client()` raises `HTTPException(500, ...)` as usual, but on `/index/message` and `/search` that call sits inside a `try` block whose `except Exception as e` re-raises everything as `503` with the original message as `detail`. A missing key therefore surfaces as `503 {"detail": "500: OPENAI_API_KEY is not configured"}`. + +The operational consequence: **a `503` from these two endpoints does not reliably mean Weaviate is down.** It may equally be an OpenAI misconfiguration. Read the `detail` string before concluding which dependency has failed. + +The key is read **per request** inside `_openai_client()`, not once at import time: + +```python +def _openai_client(): + api_key = os.environ.get("OPENAI_API_KEY") + if not api_key: + raise HTTPException(status_code=500, detail="OPENAI_API_KEY is not configured") + if OpenAI is None: + raise HTTPException(status_code=500, detail="openai package is not installed") + + return OpenAI(api_key=api_key) +``` + +Two operational consequences follow: + +1. **The service starts successfully with no API key.** Misconfiguration is not caught at boot; it surfaces as `500`s on first LLM traffic. There is no startup validation to rely on. +2. **A new `OpenAI` client is constructed on every LLM request.** There is no shared client and therefore no connection pooling or client-level reuse across requests. + +The Weaviate endpoints call `weaviate.connect_to_local()`, which targets `localhost:8080` (HTTP) and `localhost:50051` (gRPC) by default. There is no environment variable wired to override the Weaviate host in the current implementation — see [Known operational risks](#known-operational-risks). + +## Health checking + +```python +@app.get("/health") +def health_check(): + return {"status": "ok"} +``` + +`/health` returns `200` with body `{"status": "ok"}`. + +### Semantics for orchestrators + +**`/health` is a liveness probe, not a readiness probe.** It checks exactly one thing: that the ASGI process is running and able to serve a request. It performs no dependency checks whatsoever. + +Specifically, `/health` returns `200` when: + +- `OPENAI_API_KEY` is unset or invalid — the handler never calls `_openai_client()`, so the missing key is never detected. This behaviour is pinned by `test_health_works_without_api_key` in [tests/test_health.py](../tests/test_health.py). +- Weaviate is unreachable, unhealthy, or has never been started. +- The OpenAI API is down, rate-limiting the service, or the account is out of quota. + +This is a deliberate property, and it is the correct behaviour for a liveness probe: an orchestrator must not restart a process because a *third-party* dependency is degraded. Restarting fixes nothing and turns a partial outage into a crash loop. + +It also means **`/health` returning `200` does not mean the service can do useful work.** Do not treat it as a readiness signal, and do not gate a deploy on it alone. + +Recommended orchestrator wiring: + +```yaml +livenessProbe: + httpGet: + path: /health + port: 8000 + initialDelaySeconds: 5 + periodSeconds: 10 + failureThreshold: 3 +``` + +Do **not** configure `/health` as a `readinessProbe` if your intent is "can this pod serve LLM traffic" — it will report ready with a missing API key and a dead Weaviate. There is no endpoint in the current implementation that verifies dependency health. Until one exists, readiness must be inferred from error-rate monitoring on the real endpoints (see [Monitoring](#monitoring)). + +## The Weaviate dependency + +Two endpoints depend on Weaviate: `POST /index/message` (writes) and `GET /search` (reads). The other three do not touch it at all. + +Both follow the same connect/operate/close pattern: + +```python +try: + client = weaviate.connect_to_local() +except Exception: + raise HTTPException(status_code=503, detail="Weaviate connection failed") + +try: + ... +except Exception as e: + raise HTTPException(status_code=503, detail=str(e)) +finally: + client.close() +``` + +### Connection lifecycle + +A **fresh Weaviate connection is opened and closed on every request**. There is no connection pool and no persistent client. `connect_to_local()` performs a startup handshake including a gRPC health check, so each request pays that setup cost. This is the single largest fixed overhead on the Weaviate path and the first thing to change if `/search` latency becomes a problem. + +The `finally: client.close()` guarantees the connection is released on both the success and failure paths, so a Weaviate outage does not leak sockets. + +### Behaviour when Weaviate is down + +| Condition | Endpoint | Response | +| --------------------------------------------- | ---------------- | -------------------------------------------------------- | +| Weaviate unreachable (connect fails) | `/index/message` | `503` — `{"detail": "Weaviate connection failed"}` | +| Weaviate unreachable (connect fails) | `/search` | `503` — `{"detail": "Weaviate connection failed"}` | +| Connects, then fails mid-operation | `/index/message` | `503` — `detail` is the stringified exception | +| Connects, then fails mid-operation | `/search` | `503` — `detail` is the stringified exception | +| Connects, `Message` collection does not exist | `/search` | `200` — `{"results": []}` | +| Connects, `Message` collection does not exist | `/index/message` | `200` — the collection is **created**, then written to | + +Two asymmetries are worth internalising: + +**`/search` fails open on a missing collection.** If the `Message` collection does not exist, `/search` returns `200` with an empty result list rather than an error. From the caller's perspective a never-indexed corpus is indistinguishable from a genuine zero-hit query. An empty `/search` response is therefore *not* evidence that the index is healthy. + +**`/index/message` creates the collection on demand.** The first successful index call after a fresh Weaviate deployment implicitly creates `Message`. There is no separate schema migration step to run, but it also means an accidental point at an empty Weaviate silently starts building a new index rather than failing loudly. + +**Errors are surfaced verbatim.** On the mid-operation failure path, `detail=str(e)` returns the raw exception message to the caller. Treat `503` bodies from these endpoints as internal diagnostic data and do not render them directly in a user-facing surface. + +### Blast radius + +A Weaviate outage degrades search and indexing only. `/chat`, `/transfers/analyse`, and `/proposals/summarise` continue to serve normally because they never open a Weaviate connection. Availability of the two dependency groups should be tracked separately; a single aggregate error rate for the service will obscure which half is broken. + +Note that both Weaviate endpoints *also* call OpenAI, for embeddings. `/index/message` and `/search` therefore need **both** dependencies healthy — and because the embedding call happens inside the broadly-caught `try` block, an OpenAI failure on these two endpoints is also reported as `503`, not `500`. The status code alone cannot tell the two dependencies apart here; only the `detail` string can. + +## Scaling characteristics + +**The service is stateless.** No session state, no in-process cache, no cross-request coordination, no sticky-session requirement. Scale horizontally by adding replicas behind any load balancer; scale vertically with `--workers`. Both are safe. + +**Latency is dominated by upstream calls.** Per-request timeouts as configured in code: + +| Endpoint | Upstream call | Timeout | +| ---------------------- | -------------------------------- | --------- | +| `/chat` | chat completion (`gpt-4o-mini`) | 30s | +| `/transfers/analyse` | chat completion (`gpt-4o-mini`) | 10s | +| `/proposals/summarise` | chat completion (`gpt-4o-mini`) | 10s | +| `/index/message` | embedding + Weaviate write | **none** | +| `/search` | embedding + Weaviate query | **none** | + +The embedding calls in `/index/message` and `/search` pass no `timeout` argument, so they fall back to the OpenAI client default (10 minutes). Weaviate operations are likewise untimed. **A hung upstream on either Weaviate endpoint can occupy a thread-pool slot far longer than any of the chat endpoints can.** Under a partial OpenAI degradation, this is the failure mode most likely to exhaust worker capacity. Set an aggressive server-side or proxy-level request timeout in front of the service to bound it. + +**Capacity is bounded by the upstream rate limit, not by the service.** Because there is no in-process concurrency limit on outbound calls, adding replicas multiplies the request rate you present to the OpenAI API. Past a certain replica count, additional capacity converts a latency problem into a `429` problem. Size replicas against your OpenAI organisation's rate limits, and note that the service does not currently retry or back off on `429` — the error propagates to the caller as a `500`. + +**The rule-based short-circuit is the one free scaling win.** In `/transfers/analyse`, transfers above the threshold are resolved without an LLM call at all: + +```python +_HIGH_VALUE_THRESHOLD = 10_000.0 + +if request.amount > _HIGH_VALUE_THRESHOLD: + return TransferAnalyseResponse( + flagged=True, + reason=f"Amount {request.amount} XLM exceeds {_HIGH_VALUE_THRESHOLD} XLM threshold", + confidence=0.99, + ) +``` + +These requests are pure CPU, return in microseconds, cost nothing, and cannot be affected by an OpenAI outage. `test_high_value_transfer_is_flagged_without_llm_call` in [tests/test_transfers.py](../tests/test_transfers.py) asserts the LLM is never called on this path. Note the boundary is strict `>`: an amount of exactly `10_000.0` takes the LLM path. + +## Cost controls on the LLM path + +**There are no rate limits, quotas, spend caps, caching, or per-caller throttling anywhere in the service.** This is a known gap, recorded here so it can be planned for rather than discovered during an incident. + +What exists today: + +- **Per-request timeouts** on the three chat endpoints (30s / 10s / 10s), which bound the duration of a single call but not the number of calls. +- **The `/transfers/analyse` high-value short-circuit**, which avoids an LLM call for large transfers. +- **A cheap model choice** — `gpt-4o-mini` for all chat completions and `text-embedding-3-small` for all embeddings. See [concepts-prompts-and-models.md](concepts-prompts-and-models.md) for the per-endpoint model detail and how to change it safely. +- **Bounded response size** on `/search`, which caps results at `limit=5`. + +What does not exist: + +- No rate limiting of any kind — no per-IP, per-caller, per-conversation, or global request cap. Any client that can reach the service can issue unbounded LLM calls. +- No authentication or authorisation on any endpoint. There is no API key check, no bearer token, and no caller identity. Combined with the absence of rate limiting, **any party with network reach to the service can spend against the OpenAI account directly.** +- No `max_tokens` on any completion call, so output length — and therefore per-call cost — is bounded only by the model's own limit. +- No cap on input size. `ChatRequest.message`, `IndexMessageRequest.content`, and the `/search` `q` parameter are unbounded strings. A large payload becomes a large token bill. +- No caching. Identical `/search` queries re-embed on every call; identical `/chat` messages re-complete on every call. +- No retry or backoff, so a `429` from OpenAI surfaces as a `500` rather than being absorbed. +- No cost or token-usage metric is recorded. `response.usage` is available on every OpenAI response and is discarded. + +### Required mitigations + +Until controls exist in the application, they must be enforced in the deployment: + +1. **Do not expose the service to the public internet.** Bind it to an internal network and let only the backend reach it. This is the single most important control, because it is the only thing standing between an unauthenticated endpoint and an unbounded bill. +2. **Enforce rate limits at the ingress/gateway layer** — per-caller and global — since the application enforces none. +3. **Set a hard monthly spend cap and usage alerts in the OpenAI dashboard.** This is the only true backstop against runaway spend. +4. **Bound request body size at the proxy** to cap per-call token cost. +5. **Set an aggressive proxy request timeout** to cover the untimed embedding and Weaviate calls. + +## Monitoring + +There is no metrics endpoint, no structured logging, and no tracing in the service. Observability must come from the layer in front of it. Track at minimum: + +- **Rate of `500`s on `/chat`, `/transfers/analyse`, and `/proposals/summarise`** — the signal for a missing/invalid `OPENAI_API_KEY`, an OpenAI outage, or `429` rate limiting, none of which `/health` will show. +- **Rate of `503`s on `/index/message` and `/search`** — a combined Weaviate *and* OpenAI failure signal, again invisible to `/health`. Because both dependencies collapse into the same status code here, alert on the code but triage on the `detail` string. +- **p99 latency on `/index/message` and `/search`** separately from the chat endpoints, because these are the untimed paths. +- **OpenAI spend and token usage**, from the provider dashboard, since the service records neither. + +For the wider platform's metric conventions see [docs/observability.md](../../../docs/observability.md). + +## Known operational risks + +Recorded so they are tracked rather than rediscovered: + +| # | Risk | Impact | Suggested mitigation | +| - | ---- | ------ | -------------------- | +| 1 | No authentication on any endpoint | Anyone with network reach can spend against the OpenAI account | Network isolation now; auth in the application | +| 2 | No rate limiting or spend cap in the application | Unbounded cost exposure under abuse or a client bug | Gateway rate limits + OpenAI dashboard spend cap | +| 3 | `/health` never checks dependencies | A pod reports healthy while unable to serve any real request | Treat as liveness only; add a separate readiness endpoint | +| 4 | No startup validation of `OPENAI_API_KEY` | Misconfiguration deploys cleanly and fails on first traffic | Validate at startup and fail fast | +| 5 | Weaviate host is not configurable | `connect_to_local()` hardcodes localhost; Weaviate must be co-located | Wire host/port to environment variables | +| 6 | No timeout on embedding or Weaviate calls | A hung upstream holds a worker thread for up to 10 minutes | Pass explicit timeouts; bound at the proxy | +| 7 | New `OpenAI` client and new Weaviate connection per request | Avoidable per-request setup latency | Reuse clients across requests via app lifespan | +| 8 | `503` bodies leak raw exception text | Internal detail exposed to callers | Log the exception; return a generic message | +| 8a | The broad `except Exception` on both Weaviate endpoints swallows `HTTPException`, remapping a `500` to a `503` | A missing API key is indistinguishable from a Weaviate outage by status code | Re-raise `HTTPException` unchanged; catch only Weaviate errors | +| 9 | `/search` returns `200` `{"results": []}` when the collection is missing | An unindexed corpus is indistinguishable from no matches | Distinguish the two states in the response | +| 10 | No `max_tokens` and no input size cap | Per-call cost is unbounded | Set `max_tokens`; validate input length | + +## Related documents + +- [Prompts and model configuration](concepts-prompts-and-models.md) — models per endpoint, prompts, and output parsing +- [Testing guide](testing.md) — running the suite without a real model or vector store +- [Chat API](api-chat.md) — `POST /chat` request/response detail +- [Transfers analyse API](api-transfers-analyse.md) — `POST /transfers/analyse` detail +- [Proposals summarise API](api-proposals-summarise.md) — `POST /proposals/summarise` detail +- [Index and search API](api-index-search.md) — `POST /index/message` and `GET /search` detail +- [Weaviate schema](contracts-weaviate-schema.md) — the `Message` collection +- [RAG search architecture](concepts-rag-search-architecture.md) — how indexing and retrieval fit together +- [Operator runbook](../../../docs/runbook.md) — platform-wide failure modes and incident response +- [Observability](../../../docs/observability.md) — platform metric and dashboard conventions diff --git a/apps/ai_agent/docs/testing.md b/apps/ai_agent/docs/testing.md new file mode 100644 index 0000000..c2751e3 --- /dev/null +++ b/apps/ai_agent/docs/testing.md @@ -0,0 +1,416 @@ +# AI agent testing guide + +How the `ai_agent` pytest suite is configured, what each fixture in [tests/conftest.py](../tests/conftest.py) patches, and how to add a test for a new endpoint without ever calling a real model or vector store. + +## Running the suite + +All commands run from `apps/ai_agent`. + +```bash +# install dev dependencies (pytest, pytest-mock, pytest-cov, httpx) +uv sync --extra dev + +# run everything +uv run pytest + +# one module +uv run pytest tests/test_chat.py + +# one test +uv run pytest tests/test_chat.py::test_correct_model_used + +# quieter, stop on first failure +uv run pytest -q -x +``` + +The suite is fully hermetic: **no test makes a network call, and none requires a running Weaviate or a real OpenAI key.** Every external dependency is patched. A test that hangs or hits the network is a bug in that test, not a missing service. + +## Test configuration + +From [pyproject.toml](../pyproject.toml): + +```toml +[tool.pytest.ini_options] +pythonpath = ["."] +testpaths = ["tests"] +addopts = "--cov=. --cov-report=term-missing --cov-config=pyproject.toml" + +[tool.coverage.run] +omit = ["tests/*"] +``` + +What each line does: + +- **`pythonpath = ["."]`** puts `apps/ai_agent` on `sys.path`, which is what makes the bare `from main import app` import work. Without it every test module fails at import. It is also why pytest must be invoked from `apps/ai_agent` rather than the repo root. +- **`testpaths = ["tests"]`** scopes collection to `tests/`, so a bare `pytest` never wanders into other packages. +- **`addopts`** applies coverage flags automatically — plain `pytest` already produces a coverage report; you never pass `--cov` by hand. + +Dev dependencies that matter to the suite: `pytest`, `pytest-mock` (provides the `mocker` fixture the conftest fixtures are built on), `pytest-cov`, and `httpx` (required by FastAPI's `TestClient`). + +## Fixtures + +[tests/conftest.py](../tests/conftest.py) defines four fixtures, available to every test module without import. + +### `set_openai_key` — autouse + +```python +@pytest.fixture(autouse=True) +def set_openai_key(monkeypatch: pytest.MonkeyPatch) -> None: + """Ensure OPENAI_API_KEY is always set so _openai_client() doesn't 500.""" + monkeypatch.setenv("OPENAI_API_KEY", "test-key") +``` + +- **Patches**: the `OPENAI_API_KEY` environment variable. +- **Default**: the literal string `"test-key"`. +- **Autouse**: applies to **every test in the suite** — you never request it by name. + +This exists because `_openai_client()` reads the key on every call and raises `HTTPException(500, "OPENAI_API_KEY is not configured")` when it is absent. Without this fixture, every LLM endpoint test would return `500` on a developer machine with no key set, and results would differ between local runs and CI. The value is never used to authenticate — the OpenAI client is always patched out — it only has to be non-empty. + +`monkeypatch` reverts the variable after each test, so there is no leakage between tests. + +**To test the missing-key path, delete the variable inside the test**, which overrides the autouse fixture for that test only: + +```python +def test_missing_api_key_returns_500(monkeypatch, client): + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + response = client.post("/chat", json=_BASE_BODY) + assert response.status_code == 500 +``` + +`raising=False` keeps the call safe when the variable is already gone. + +### `client` + +```python +@pytest.fixture() +def client() -> TestClient: + """FastAPI TestClient for the main app.""" + from main import app + + return TestClient(app) +``` + +- **Patches**: nothing. It constructs a real `TestClient` around the real `app`. +- **Returns**: a `fastapi.testclient.TestClient` bound to the app from `main.py`. + +`TestClient` dispatches requests in-process through the ASGI app — no socket is opened and no server is started. Routing, Pydantic request validation, `response_model` validation, and `HTTPException` handling all run exactly as in production, so status codes and bodies are trustworthy. + +The `from main import app` import is deliberately *inside* the fixture body rather than at module top level, so the app is imported when the fixture first runs rather than at conftest collection time. + +Note this fixture is function-scoped: each test gets a fresh `TestClient` over the same module-level `app` object. + +### `mock_openai` + +```python +@pytest.fixture() +def mock_openai(mocker): + """Patch the OpenAI client used inside main.py.""" + return mocker.patch("main.OpenAI") +``` + +- **Patches**: the `OpenAI` **class** as bound in `main`'s namespace (`main.OpenAI`). +- **Returns**: the `MagicMock` that replaced the class. + +The patch target is `main.OpenAI`, not `openai.OpenAI`, because `main.py` does `from openai import OpenAI` — the name must be patched where it is *used*, not where it is defined. + +Because the class is patched, `_openai_client()` still runs for real: it still checks `OPENAI_API_KEY` (supplied by the autouse fixture), still checks `OpenAI is None`, and then calls `OpenAI(api_key=api_key)` — which now returns `mock_openai.return_value`. **That means the instance your endpoint uses is `mock_openai.return_value`**, and that is the object to configure: + +```python +def test_valid_request_returns_reply(mock_openai, client): + mock_client = mock_openai.return_value + mock_client.chat.completions.create.return_value = _fake_chat_reply("Hello from Clicked AI!") +``` + +By default — with nothing configured — every attribute access returns a fresh `MagicMock`. That is enough to prove no real call was made, but not enough for an endpoint that reads `response.choices[0].message.content`: a bare `MagicMock` there fails `ChatResponse` validation. Configure a return value whenever the handler reads the response. + +The mock also records calls, which is how the suite asserts on prompts and model IDs without a real request: + +```python +call_args = mock_client.chat.completions.create.call_args +assert call_args[1]["model"] == "gpt-4o-mini" +messages = call_args[1]["messages"] +assert messages[0]["role"] == "system" +``` + +### `mock_weaviate` + +```python +@pytest.fixture() +def mock_weaviate(mocker): + """Patch weaviate.connect_to_local used inside main.py.""" + return mocker.patch("main.weaviate.connect_to_local") +``` + +- **Patches**: the `connect_to_local` function on the `weaviate` module as seen from `main`. +- **Returns**: the `MagicMock` that replaced the function. + +`main.py` does `import weaviate` and calls `weaviate.connect_to_local()`, so the attribute on the module object is the correct target. The connected client your endpoint receives is `mock_weaviate.return_value`. + +By default it returns a `MagicMock`, so `client.collections.exists(...)` returns a truthy `MagicMock` rather than a real boolean. For `/search`, whose branch on `collections.exists("Message")` decides between an empty result and a query, set it explicitly: + +```python +def test_search_returns_empty_when_collection_missing(mock_weaviate, mock_openai, client): + mock_weaviate.return_value.collections.exists.return_value = False + ... +``` + +To simulate an outage, give the mock a `side_effect` so the `try`/`except` around `connect_to_local()` fires: + +```python +mock_weaviate.side_effect = Exception("connection refused") +# endpoint now returns 503 {"detail": "Weaviate connection failed"} +``` + +The default `MagicMock` also satisfies the `finally: client.close()` in both Weaviate endpoints, so no extra setup is needed for teardown. + +### A note on existing modules + +The fixtures above are the intended approach for new tests, but **the suite is not uniform today**. Only [tests/test_chat.py](../tests/test_chat.py) uses `client` and `mock_openai` throughout. [tests/test_search.py](../tests/test_search.py), [tests/test_transfers.py](../tests/test_transfers.py), and [tests/test_health.py](../tests/test_health.py) instead build a module-level `TestClient(app)` and use `unittest.mock.patch` inline — often patching `main._openai_client` (the helper) rather than `main.OpenAI` (the class). + +Both styles work. Patching `main._openai_client` bypasses the real helper entirely, which is why `test_transfers.py` can assert `mock_openai.assert_not_called()` to prove the high-value rule never reaches the LLM. Prefer the conftest fixtures in new code — they are shorter and consistent — but do not treat the older modules as broken. + +## Worked example: adding a test for a new endpoint + +Suppose a `POST /proposals/classify` endpoint is added to `main.py`, calling `gpt-4o-mini` in JSON mode and returning a typed `category` plus `confidence`, with `category` clamped to `"treasury"` when the model returns something invalid. + +Create `tests/test_classify.py`. There is no fixture import and no conftest boilerplate — fixtures are injected by name. + +```python +"""Unit tests for POST /proposals/classify.""" + +import json +from unittest.mock import MagicMock + +_BASE_BODY = { + "title": "Fund the Q3 audit", + "description": "Engage an external auditor for the treasury contracts.", + "amount": 2500.0, +} + + +def _fake_json_response(payload: dict): + """Shape a MagicMock like an OpenAI chat completion carrying `payload` as JSON.""" + msg = MagicMock() + msg.content = json.dumps(payload) + choice = MagicMock() + choice.message = msg + resp = MagicMock() + resp.choices = [choice] + return resp + + +def test_returns_classification(mock_openai, client): + mock_client = mock_openai.return_value + mock_client.chat.completions.create.return_value = _fake_json_response( + {"category": "treasury", "confidence": 0.87} + ) + + response = client.post("/proposals/classify", json=_BASE_BODY) + + assert response.status_code == 200 + data = response.json() + assert data["category"] == "treasury" + assert data["confidence"] == 0.87 + + +def test_uses_expected_model_and_json_mode(mock_openai, client): + mock_client = mock_openai.return_value + mock_client.chat.completions.create.return_value = _fake_json_response( + {"category": "treasury", "confidence": 0.5} + ) + + client.post("/proposals/classify", json=_BASE_BODY) + + kwargs = mock_client.chat.completions.create.call_args[1] + assert kwargs["model"] == "gpt-4o-mini" + assert kwargs["response_format"] == {"type": "json_object"} + + +def test_prompt_includes_proposal_fields(mock_openai, client): + mock_client = mock_openai.return_value + mock_client.chat.completions.create.return_value = _fake_json_response( + {"category": "treasury", "confidence": 0.5} + ) + + client.post("/proposals/classify", json=_BASE_BODY) + + prompt = mock_client.chat.completions.create.call_args[1]["messages"][0]["content"] + assert "Fund the Q3 audit" in prompt + assert "2500.0" in prompt + + +def test_invalid_category_is_clamped(mock_openai, client): + """A category outside the allowed set must degrade, not leak through.""" + mock_client = mock_openai.return_value + mock_client.chat.completions.create.return_value = _fake_json_response( + {"category": "nonsense", "confidence": 0.4} + ) + + response = client.post("/proposals/classify", json=_BASE_BODY) + + assert response.status_code == 200 + assert response.json()["category"] == "treasury" + + +def test_missing_confidence_defaults_to_zero(mock_openai, client): + mock_client = mock_openai.return_value + mock_client.chat.completions.create.return_value = _fake_json_response( + {"category": "treasury"} + ) + + response = client.post("/proposals/classify", json=_BASE_BODY) + + assert response.status_code == 200 + assert response.json()["confidence"] == 0.0 + + +def test_missing_title_returns_422(client): + """Pydantic rejects the body before any model call — no mock needed.""" + response = client.post( + "/proposals/classify", + json={"description": "no title", "amount": 100.0}, + ) + assert response.status_code == 422 + + +def test_missing_api_key_returns_500(monkeypatch, client): + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + response = client.post("/proposals/classify", json=_BASE_BODY) + assert response.status_code == 500 +``` + +The pattern generalises to any endpoint: + +1. **Request `mock_openai` and/or `mock_weaviate`** for endpoints with external calls. Omit both for pure-validation tests — `test_missing_title_returns_422` never reaches a client, so mocking it would be noise. +2. **Never request `set_openai_key`** — it is autouse. +3. **Shape the mock response to what the handler reads.** For chat completions that is `response.choices[0].message.content`; for embeddings it is `response.data[0].embedding`. A local helper like `_fake_json_response` keeps this in one place. +4. **Assert on `call_args`** to pin the model, JSON mode, and prompt contents without a real call. +5. **Cover the degradation paths, not just the happy path.** Missing keys, invalid enum values, and malformed JSON are where the parsing layer earns its keep — see [concepts-prompts-and-models.md](concepts-prompts-and-models.md#output-parsing-and-validation) for what each endpoint does with bad output. +6. **Cover the `422` and `500` boundaries** — Pydantic rejection and missing API key. + +For a Weaviate-backed endpoint, add `mock_weaviate` and shape the embedding response too: + +```python +def test_search_returns_hits(mock_weaviate, mock_openai, client): + embedding = MagicMock() + embedding.embedding = [0.1] * 1536 + embed_result = MagicMock() + embed_result.data = [embedding] + mock_openai.return_value.embeddings.create.return_value = embed_result + + obj = MagicMock() + obj.properties = { + "messageId": "msg-1", + "conversationId": "conv-abc", + "senderId": "user-1", + "content": "payment sent", + } + query_result = MagicMock() + query_result.objects = [obj] + + wv = mock_weaviate.return_value + wv.collections.exists.return_value = True + wv.collections.get.return_value.query.near_vector.return_value = query_result + + response = client.get("/search", params={"q": "payment", "conversationId": "conv-abc"}) + + assert response.status_code == 200 + assert response.json()["results"][0]["messageId"] == "msg-1" +``` + +## Coverage + +Coverage runs automatically on every invocation via `addopts`: + +```toml +addopts = "--cov=. --cov-report=term-missing --cov-config=pyproject.toml" +``` + +- **`--cov=.`** measures everything under `apps/ai_agent` — in practice `main.py`, since it is the only application module. +- **`--cov-report=term-missing`** prints the report to the terminal *and* lists the specific line numbers not executed. This is the useful part: it names the untested lines rather than just a percentage. +- **`--cov-config=pyproject.toml`** reads coverage settings from `[tool.coverage.run]`, which sets `omit = ["tests/*"]` so the test files do not inflate their own numbers. + +### Reading the report + +Output appears after the test results. This is the report from the suite as it currently stands: + +``` +................................... [100%] +=============================== tests coverage ================================ +_______________ coverage: platform win32, python 3.12.5-final-0 _______________ + +Name Stmts Miss Cover Missing +--------------------------------------- +main.py 120 22 82% 83, 183-228, 267-268, 274 +--------------------------------------- +TOTAL 120 22 82% +35 passed +``` + +- **`Stmts`** — executable statements measured (not physical lines; blanks, comments, and most `def` bodies-as-declarations are excluded). +- **`Miss`** — statements never executed by any test. +- **`Cover`** — `(Stmts - Miss) / Stmts`. +- **`Missing`** — the line numbers that were missed, as individual lines and ranges. Open `main.py` at those lines to see exactly what is untested. + +Reading the current report: **`183-228` is the entire body of `index_message`**, which is the coverage gap described below. The rest is minor — `83` is the `OpenAI is None` guard in `_openai_client()` (unreachable while the package is installed), `267-268` is the exception branch in `/search`, and `274` is the `uvicorn.run(...)` line under `if __name__ == "__main__"`, which never executes under pytest. + +Closing the `/index/message` gap is worth roughly 38 statements, taking the module from 82% to around 97%. + +A useful additional report when hunting gaps interactively: + +```bash +uv run pytest --cov-report=html +open htmlcov/index.html +``` + +This renders each source line green (covered) or red (missed), which is faster to scan than a line-number list. + +Note that coverage is measured but **not enforced** — there is no `fail_under` setting, so a drop in coverage will not fail the build. Read the `Missing` column rather than relying on a gate. + +## Known gap: `/index/message` has no test module + +The `tests/` directory contains `test_chat.py`, `test_health.py`, `test_proposals.py`, `test_search.py`, and `test_transfers.py`. **There is no `test_index.py`, and no test anywhere in the suite exercises `POST /index/message`.** + +This is the single largest coverage gap in the service, and it is a good first contribution — the fixtures needed already exist, and `test_search.py` provides a close template since it patches the same two dependencies. + +`/index/message` is also the most complex endpoint to test properly, because it is the only one that both embeds *and* writes, with a branch on whether the object already exists. The cases worth covering: + +| Case | Setup | Expected | +| ---- | ----- | -------- | +| Weaviate unreachable | `mock_weaviate.side_effect = Exception(...)` | `503`, `{"detail": "Weaviate connection failed"}` | +| `Message` collection missing | `collections.exists.return_value = False` | Collection is **created** via `collections.create(name="Message")`, then written | +| New message (insert path) | `collection.data.exists.return_value = False` | `collection.data.insert` called; `replace` not called | +| Existing message (upsert path) | `collection.data.exists.return_value = True` | `collection.data.replace` called; `insert` not called | +| Correct embedding model | any success path | `embeddings.create` called with `model="text-embedding-3-small"` | +| Properties written correctly | any success path | `properties` carries `messageId`, `conversationId`, `senderId`, `content`; `uuid` is `messageId` | +| Mid-operation failure | make `data.insert` raise | `503` with the stringified exception as `detail` | +| Connection always closed | any path, success or failure | `mock_weaviate.return_value.close.assert_called_once()` | +| Missing required field | post a body without `content` | `422` | +| Missing API key | `monkeypatch.delenv("OPENAI_API_KEY", ...)` | **`503`**, not `500` — see below | + +Watch the last row: it is the one case here that does not behave the way the other endpoints do. `_openai_client()` raises `HTTPException(500, ...)`, but on this endpoint that call sits inside the `try` whose `except Exception as e` re-raises everything as `503`, with the original message carried through as `detail`. The assertion to write is: + +```python +def test_missing_api_key_returns_503(monkeypatch, mock_weaviate, client): + """The broad except on this endpoint remaps the helper's 500 to a 503.""" + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + mock_weaviate.return_value.collections.exists.return_value = True + + response = client.post("/index/message", json=_BODY) + + assert response.status_code == 503 + assert "OPENAI_API_KEY" in response.json()["detail"] +``` + +`GET /search` has the identical structure and the identical behaviour. + +The `close()` assertion is worth including on both a success and a failure case — it is the only guard against a Weaviate outage leaking connections, and it is currently unverified. See [operations.md](operations.md#the-weaviate-dependency) for the connection lifecycle this protects. + +## Related documents + +- [Operations guide](operations.md) — running the service, health checks, dependencies +- [Prompts and model configuration](concepts-prompts-and-models.md) — prompts, models, and the parsing behaviour these tests pin +- [Pydantic models](contracts-pydantic-models.md) — request/response shapes driving the `422` cases +- [Weaviate schema](contracts-weaviate-schema.md) — the `Message` collection the index tests would assert against +- [Index and search API](api-index-search.md) — the endpoint behaviour behind the coverage gap +- [Chat API](api-chat.md) · [Transfers analyse API](api-transfers-analyse.md) · [Proposals summarise API](api-proposals-summarise.md) diff --git a/contracts/docs/contracts-errors.md b/contracts/docs/contracts-errors.md new file mode 100644 index 0000000..283270a --- /dev/null +++ b/contracts/docs/contracts-errors.md @@ -0,0 +1,610 @@ +# Contract error and panic reference + +Every failure condition across the three Soroban contracts — `token_transfer`, `group_treasury`, and `proposals` — grouped by function: what triggers it, what the caller observes, and how a client should present it. + +This reflects the implementation on Soroban SDK 22.0.0, pinned in [contracts/Cargo.toml](../Cargo.toml). + +## How failure works in these contracts + +None of the three contracts define a `#[contracterror]` enum. **Every failure is a `panic!` with a string message, or a `.expect()` on a missing storage read.** This has direct consequences for callers: + +- **There are no stable numeric error codes.** A panic in a Soroban contract traps the host and the invocation aborts; the panic string is a debug aid, not part of the ABI. In release builds it is generally not recoverable from the transaction result, and it must never be parsed programmatically. +- **Failures are all-or-nothing.** A trapped invocation reverts every storage write and discards every event published during that call. There are no partial applications and no cleanup to perform — see [Atomicity](#atomicity-and-state-on-failure). +- **Clients must map errors by context, not by message.** Because the string is unreliable at the boundary, the frontend has to infer the cause from what it *knows about the call it made* — the function invoked and the state it read beforehand. That is why the mapping guidance below is organised around pre-flight checks and simulation. + +Failures fall into three kinds, distinguished throughout this document: + +| Kind | Meaning | Typical client response | +| ---- | ------- | ----------------------- | +| **Authorization** | The caller is not permitted, or did not sign. `require_auth()` failures and membership checks. | Explain who *is* permitted. Never retry blindly. | +| **Validation** | The arguments are unacceptable in isolation — non-positive amounts, expiry in the past. | Fix client-side before submitting; these are preventable. | +| **State-machine** | The arguments are fine but the contract is not in a state that permits the action — voting twice, approving after expiry, executing an unfinalised proposal. | Re-read on-chain state and update the UI; the action may become valid later, or never. | + +A fourth category, **initialization**, covers `.expect("not initialized")` reads. These indicate a deployment fault rather than a user error and should surface as a system error, not a user-facing validation message. + +--- + +## `token_transfer` + +Source: [contracts/contracts/token_transfer/src/lib.rs](../contracts/token_transfer/src/lib.rs). API reference: [api-token-transfer.md](api-token-transfer.md). + +### `initialize(env, admin, token_contract)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `DataKey::Admin` already present in instance storage | State-machine | `already initialized` | + +```rust +if env.storage().instance().has(&DataKey::Admin) { + panic!("already initialized"); +} +``` + +**Caller observes**: the invocation traps; no storage is written. + +**Note there is no authorization on this function.** `initialize` calls no `require_auth()`, so on a freshly deployed, uninitialized contract *anyone* can call it and set themselves as admin. Deployment and initialization must therefore happen in the same atomic step — see [api-deployment-invocation.md](api-deployment-invocation.md). The `already initialized` guard is the only thing preventing a takeover after the fact. + +**Client presentation**: this is an operator action, not a user action. Surface as "Contract is already initialized" in tooling; it should never reach an end user. + +### `transfer(env, from, to, amount, memo)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `amount <= 0` | Validation | `amount must be positive` | +| `from` did not authorize the call | Authorization | host auth failure (no contract message) | +| `DataKey::TokenContract` unset | Initialization | `not initialized` | +| Underlying SEP-41 token rejects the transfer | Delegated | token contract's own failure | + +Order matters: the amount check runs **before** `from.require_auth()`, so a non-positive amount fails without ever prompting the user to sign. + +```rust +if amount <= 0 { + panic!("amount must be positive"); +} +from.require_auth(); +``` + +The most common real-world failure is the last row: **insufficient balance is not checked by this contract at all.** `token_transfer` is a thin router; it calls `token.transfer(&from, &to, &amount)` and the SEP-41 token contract enforces balance. The failure therefore originates in a sub-invocation, and the panic message — if any is visible — belongs to the token contract, not this one. + +**Client presentation**: + +- `amount <= 0` — prevent entirely with client-side validation; never let it reach the chain. +- Auth failure — most often the user rejected the Freighter prompt. Present as a cancellation, not an error. +- Insufficient balance — check the balance before submitting and block the action with a clear message. If it still fails at submit time (balance changed between check and submit), present "Insufficient balance to complete this transfer". +- `not initialized` — system error; the deployment is broken. + +### `balance(env, address)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `DataKey::TokenContract` unset | Initialization | `not initialized` | + +Read-only, no authorization. Can also fail if the underlying token contract's `balance` call fails. + +**Client presentation**: treat a failure here as "balance unavailable" and render a placeholder rather than `0` — showing zero for an unreadable balance is misleading. + +### `token_contract(env)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `DataKey::TokenContract` unset | Initialization | `not initialized` | + +Read-only, no authorization. Fails only on an uninitialized contract. + +### `set_token_contract(env, new_token)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `DataKey::Admin` unset | Initialization | `not initialized` | +| Caller is not the admin | Authorization | host auth failure | + +```rust +let admin: Address = env.storage().instance().get(&DataKey::Admin) + .expect("not initialized"); +admin.require_auth(); +``` + +The admin is loaded from storage first, so an uninitialized contract fails with `not initialized` before any auth check. + +**Client presentation**: admin-only tooling. Non-admin users should never see this function in the UI. + +### `upgrade(env, new_wasm_hash)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `DataKey::Admin` unset | Initialization | `not initialized` | +| Caller is not the admin | Authorization | host auth failure | +| `new_wasm_hash` is not an installed wasm | Host | host failure from `update_current_contract_wasm` | + +Same admin gate as `set_token_contract`. The third row is a host-level failure: the hash must correspond to wasm already installed on the network. + +**Client presentation**: operator tooling only. + +--- + +## `group_treasury` + +Source: [contracts/contracts/group_treasury/src/lib.rs](../contracts/group_treasury/src/lib.rs). + +Every admin-gated function routes through one helper, so the same two failures recur across all of them: + +```rust +fn require_admin(env: &Env) -> Address { + let admin: Address = env.storage().instance().get(&DataKey::Admin) + .expect("not initialized"); + admin.require_auth(); + admin +} +``` + +### `initialize(env, admin, _token, threshold)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| Already initialized | State-machine | `already initialized` | +| `threshold == 0` | Validation | `threshold must be at least 1` | + +As with `token_transfer::initialize`, **there is no `require_auth()`** — initialization is a deployment-time race and must be atomic with deployment. + +The `threshold` guard prevents a treasury where zero approvals suffice. Note it only enforces a lower bound: a `threshold` **greater than the eventual member count** is accepted, producing a treasury where no withdraw proposal can ever reach approval. Members are added after initialization, so the contract cannot validate this at init time. Operator tooling should check it. + +**Client presentation**: operator error. "Approval threshold must be at least 1." + +### `get_threshold(env)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| Not initialized | Initialization | `not initialized` | + +Read-only, no authorization. + +### `add_member(env, member)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| Not initialized | Initialization | `not initialized` | +| Caller is not the admin | Authorization | host auth failure | +| `member` is already in the members list | State-machine | `member already exists` | + +**Client presentation**: check membership with `is_member` before offering the action, and present `member already exists` as "This address is already a member" rather than a failure. + +### `remove_member(env, member)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| Not initialized | Initialization | `not initialized` | +| Caller is not the admin | Authorization | host auth failure | +| `member` is not in the members list | State-machine | `member not found` | + +**Removing a member does not clean up their votes.** `DataKey::Vote(id, member)` entries survive removal, so a removed member's existing approvals still count toward `approvals` on open proposals. Removal also shrinks `member_count`, which changes the `blocking_minority` calculation in `reject_withdraw` for every open proposal. Clients should re-read open proposals after any membership change rather than trusting cached tallies. + +### `is_member(env, member)` / `get_members(env)` + +No failure conditions. Both use `unwrap_or_else(|| Vec::new(&env))`, so they return `false` and an empty vector respectively on an uninitialized contract rather than panicking. **Neither can be used to detect an uninitialized treasury** — use `get_threshold` for that. + +### `deposit(env, from, token, amount)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `amount <= 0` | Validation | `amount must be positive` | +| `from` did not authorize | Authorization | host auth failure | +| Token transfer into the treasury fails | Delegated | token contract's own failure | + +The amount check precedes `from.require_auth()`, so an invalid amount fails before any signing prompt. + +**Deposit is not member-gated** — any address may deposit into the treasury. This is deliberate. + +As with `token_transfer::transfer`, the depositor's balance is enforced by the SEP-41 token contract, not here. + +**Client presentation**: validate the amount client-side; present a token failure as "Insufficient balance". + +### `withdraw(env, to, token, amount)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `amount <= 0` | Validation | `amount must be positive` | +| Not initialized | Initialization | `not initialized` | +| Caller is not the admin | Authorization | host auth failure | +| Tracked balance for `token` is less than `amount` | State-machine | `insufficient funds` | +| Token transfer out fails | Delegated | token contract's own failure | + +Order is: amount check → `require_admin` → balance check → transfer. + +**This is the admin bypass path.** `withdraw` is admin-only and does **not** consult the proposal system at all — no threshold, no approvals, no member vote. The multisig flow (`propose_withdraw` → `approve_withdraw`) is a separate mechanism, and this function is not gated by it. Note that `proposals::execute_withdraw` calls *this* function via the treasury interface, which means the `proposals` contract address must itself be the treasury admin for that path to work. + +The `insufficient funds` check reads the contract's **internally tracked** `Balances` map, not the token contract's real balance. If tokens are transferred directly to the treasury address without going through `deposit`, the tracked balance understates reality and withdrawals will be refused despite the funds existing. + +**Client presentation**: "Insufficient treasury balance" — and surface the tracked balance from `balance(token)` so the number the user sees matches the number the contract checks. + +### `balance(env, token)` + +No failure conditions. Returns `0` for an unknown token or an uninitialized contract via `unwrap_or(0)`. + +**A zero return is ambiguous** — it may mean no funds, an unknown token, or an uninitialized treasury. Clients that need to distinguish these must call `get_threshold` to confirm initialization. + +### `propose_withdraw(env, proposer, to, token, amount, ttl_ledgers)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `proposer` did not authorize | Authorization | host auth failure | +| `proposer` is not a member | Authorization | `proposer is not a member` | +| `amount <= 0` | Validation | `amount must be positive` | +| Tracked balance for `token` is less than `amount` | State-machine | `insufficient funds` | + +Order: `require_auth` → membership → amount → balance. **The signing prompt appears before the membership check**, so a non-member is asked to sign a transaction that then fails. Check `is_member` client-side first to avoid this. + +The balance check happens at *proposal* time. Funds are not escrowed, so a proposal that was fundable when created may be unfundable when executed. + +`expires_at` is computed as `env.ledger().timestamp() + (ttl_ledgers as u64 * 5)` — an approximation of 5 seconds per ledger. It is a timestamp, not a ledger count, despite the parameter name. + +**Client presentation**: gate the UI on `is_member`; validate amount and balance before submitting. + +### `approve_withdraw(env, approver, proposal_id)` / `reject_withdraw(env, rejecter, proposal_id)` + +Both delegate validation to one shared helper, so **their failure conditions are identical**: + +```rust +fn require_votable(env: &Env, voter: &Address, proposal_id: u32) -> WithdrawProposal { + voter.require_auth(); + + if !Self::is_member(env.clone(), voter.clone()) { + panic!("not a member"); + } + + let proposal: WithdrawProposal = env.storage().instance() + .get(&DataKey::Proposal(proposal_id)) + .expect("proposal not found"); + + if proposal.status != ProposalStatus::Active { + panic!("proposal is not pending"); + } + if proposal.status == ProposalStatus::Expired + || env.ledger().timestamp() >= proposal.expires_at + { + panic!("proposal expired"); + } + if env.storage().instance().has(&DataKey::Vote(proposal_id, voter.clone())) { + panic!("already voted"); + } + + proposal +} +``` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| Voter did not authorize | Authorization | host auth failure | +| Voter is not a member | Authorization | `not a member` | +| No proposal with `proposal_id` | State-machine | `proposal not found` | +| Proposal status is not `Active` | State-machine | `proposal is not pending` | +| Voting window has closed | State-machine | `proposal expired` | +| This address already voted | State-machine | `already voted` | +| Threshold read fails | Initialization | `not initialized` | + +**A subtlety worth knowing when reading the messages.** The `status != Active` check runs before the expiry check, and `Expired` is not `Active`. So a proposal whose *status field* has been set to `Expired` fails with `proposal is not pending`, and the `status == Expired` half of the expiry condition is unreachable. `proposal expired` is only ever emitted for a proposal still marked `Active` whose `expires_at` has passed — the wall-clock case. Both mean "you cannot vote on this", so this does not change client behaviour; it matters only if you are matching messages during debugging. + +Note also that **the proposer's auto-approval is recorded as a vote** at creation time (`approvals: 1` and a `Vote` entry). The proposer therefore gets `already voted` if they try to approve their own proposal, and cannot reject it either. + +**Client presentation** — this is the richest mapping surface, and all six conditions are avoidable client-side: + +| Condition | Message to show | +| --------- | --------------- | +| Not a member | "Only treasury members can vote on withdrawals." | +| Proposal not found | "This proposal no longer exists." | +| Not pending | "This proposal has already been resolved." Re-read and show the final status. | +| Expired | "The voting window for this proposal has closed." | +| Already voted | "You have already voted on this proposal." Show their recorded vote. | + +Read the proposal with `get_proposal` and check `is_member` before rendering vote buttons; every one of these should be a disabled control with an explanation rather than a failed transaction. + +### `get_proposal(env, proposal_id)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| No proposal with `proposal_id` | State-machine | `proposal not found` | + +Read-only, no authorization. + +### `list_proposals(env)` / `get_pending_proposals(env)` + +No failure conditions; return an empty `Vec` when nothing matches. + +**Both iterate `1..=count`, but ids are assigned starting at `0`.** Proposal `0` — the first proposal ever created — is therefore never returned by either function, while the loop's final iteration looks up a non-existent id. The `if let Some(...)` guard means this does not panic, but a client relying on these functions will silently miss the first proposal. Fetch by id with `get_proposal` when completeness matters. + +--- + +## `proposals` + +Source: [contracts/contracts/proposals/src/lib.rs](../contracts/proposals/src/lib.rs). API reference: [api-proposals.md](api-proposals.md). Lifecycle: [concepts-proposal-lifecycle.md](concepts-proposal-lifecycle.md). + +All functions that take a `proposal_id` load it through one helper: + +```rust +fn load_proposal(env: &Env, proposal_id: u64) -> Proposal { + env.storage().instance().get(&DataKey::Proposal(proposal_id)) + .expect("proposal not found") +} +``` + +So **`proposal not found` is a possible failure of every id-taking function** in this contract and is not repeated in each table below. + +### `initialize(env, admin)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| Already initialized | State-machine | `already initialized` | +| `admin` did not authorize | Authorization | host auth failure | + +Unlike the other two contracts, this `initialize` **does** call `admin.require_auth()` — after the already-initialized guard. + +### `create_proposal(env, proposer, description, expires_at, treasury, token, to, amount)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `proposer` did not authorize | Authorization | host auth failure | +| `expires_at <= now` | Validation | `expires_at must be in the future` | +| `amount <= 0` | Validation | `amount must be positive` | + +`expires_at` is a **unix timestamp in seconds**, compared against `env.ledger().timestamp()`. Passing a duration rather than an absolute timestamp is the classic mistake here and fails immediately. + +There is no membership check — **anyone may create a proposal**. Membership is enforced later, at `execute_withdraw`. + +**Client presentation**: both validation failures are fully preventable. Compute `expires_at` as an absolute timestamp with a safety margin, and validate the amount before submitting. + +### `vote(env, voter, proposal_id, support)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `voter` did not authorize | Authorization | host auth failure | +| Proposal status is not `Active` | State-machine | `proposal is not active` | +| `now >= expires_at` | State-machine | `voting window has closed` | +| This address already voted | State-machine | `voter has already voted` | + +```rust +let vote_key = DataKey::Vote(proposal_id, voter.clone()); +if env.storage().instance().has(&vote_key) { + panic!("voter has already voted"); +} +``` + +**One vote per address per proposal, and votes cannot be changed** — there is no revoke or re-vote path. The stored value records the direction, and the mere presence of the key blocks any further vote. + +There is no membership or token-weighting check: any address that can pay the fee may vote, and every vote counts equally. + +The boundary is `now >= expires_at`, so voting is closed *at* the expiry timestamp, not one second after. + +**Client presentation**: + +| Condition | Message to show | +| --------- | --------------- | +| Not active | "Voting has closed on this proposal." Show the current status. | +| Window closed | "The voting period ended." Show the expiry time. | +| Already voted | "You have already voted." Show the recorded direction. | + +Read `get_proposal` and check the vote key before rendering vote controls. + +### `finalize_proposal(env, proposal_id)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| Proposal status is not `Active` | State-machine | `proposal already finalized` | +| `now < expires_at` | State-machine | `cannot finalize before expiry` | + +**Callable by anyone** — no `require_auth()`. This is deliberate: finalisation must not depend on a specific party being available. + +Outcome mapping once it succeeds: `yes_votes > no_votes` → `Passed`; otherwise → `Rejected`. **A tie is a rejection**, and a proposal with zero votes is rejected. + +**Client presentation**: expose finalisation only after `expires_at` has passed. `proposal already finalized` usually means someone else finalised first — treat it as success, re-read the proposal, and show the outcome rather than an error. + +### `finalize_expired_proposal(env, proposal_id)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| Proposal status is not `Active` | State-machine | `proposal not Pending` | +| `now <= expires_at` | State-machine | `proposal not expired` | + +An alternative terminal path that sets status to `Expired` instead of tallying votes. No authorization. + +**Two ways to close the same proposal.** `finalize_proposal` and `finalize_expired_proposal` are both callable on the same `Active`, past-expiry proposal, and whichever lands first wins — the other then fails with its "already finalized" equivalent. `finalize_expired_proposal` discards the vote tally entirely, so **a proposal that would have `Passed` can be closed as `Expired` instead**, permanently blocking execution. Clients should call `finalize_proposal`; `finalize_expired_proposal` is for abandoning proposals. + +The boundary differs by one: `finalize_proposal` requires `now >= expires_at`, while `finalize_expired_proposal` requires `now > expires_at`. Exactly at the expiry timestamp only `finalize_proposal` is callable. + +**Client presentation**: `proposal not Pending` and `proposal not expired` both mean the same thing to a user — "this proposal cannot be closed right now". Re-read state and show the actual status. + +### `execute_proposal(env, executor, proposal_id)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `executor` did not authorize | Authorization | host auth failure | +| Proposal status is not `Passed` | State-machine | `proposal is not in Passed state` | + +**This is the canonical "executing an unfinalised proposal" failure.** A proposal that is still `Active` — even one with overwhelming support and a passed expiry — is not `Passed` until `finalize_proposal` has run. Attempting to execute it fails with `proposal is not in Passed state`. The same message covers `Rejected`, `Expired`, and already-`Executed` proposals, so the message alone does not tell the caller which case they hit. + +Execution is otherwise unrestricted: any address may execute a `Passed` proposal. + +This function only flips the status to `Executed` and emits an event. It moves no funds — that is `execute_withdraw`. + +**Client presentation**: read the proposal and branch on the actual status rather than relying on the message. + +| Actual status | Message to show | +| ------------- | --------------- | +| `Active`, past expiry | "This proposal must be finalised before it can be executed." Offer the finalise action. | +| `Active`, before expiry | "Voting is still open until ." | +| `Rejected` | "This proposal was rejected and cannot be executed." | +| `Expired` | "This proposal expired without being finalised." | +| `Executed` | "This proposal has already been executed." | + +### `execute_withdraw(env, caller, proposal_id)` + +The most failure-prone function in the codebase — it spans authorization, state-machine, and cross-contract failures. + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| `caller` did not authorize | Authorization | host auth failure | +| Status is `Executed` | State-machine | `proposal already executed` | +| Status is not `Passed` | State-machine | `proposal not approved` | +| `caller` is not a treasury member | Authorization | `caller is not a treasury member` | +| Treasury balance is less than `proposal.amount` | State-machine | `insufficient funds` | +| The treasury `withdraw` call fails | Delegated | `group_treasury` failure (see below) | + +Checks run in that order, so the already-executed case is distinguished from the general not-approved case by its own message — the only place in these contracts where double-execution is called out specifically. + +Membership is checked against the **treasury contract stored on the proposal**, via a cross-contract call: + +```rust +let treasury_client = crate::treasury_interface::TreasuryClient::new(&env, &proposal.treasury); + +if !treasury_client.is_member(&caller.clone()) { + panic!("caller is not a treasury member"); +} +``` + +Two failure sources are easy to miss: + +- **The `treasury` address is fixed at proposal creation** and never validated. A proposal created with a wrong or non-existent treasury address fails at this cross-contract call, and the failure will look like a host error rather than a clean panic. +- **The final `treasury_client.withdraw(...)` is admin-gated inside `group_treasury`.** For this path to work, the `proposals` contract's own address must be the treasury's admin. If it is not, execution fails at the last step with an authorization failure from the *other* contract — after every check in this function has passed. This is the hardest failure here to diagnose from the client, and it is a deployment misconfiguration, not a user error. + +Note the balance is checked here and again inside `group_treasury::withdraw`; both read the same tracked `Balances` map. + +**Client presentation**: + +| Condition | Message to show | +| --------- | --------------- | +| Already executed | "This withdrawal has already been executed." | +| Not approved | "This proposal has not been approved." Show the status. | +| Not a treasury member | "Only treasury members can execute this withdrawal." | +| Insufficient funds | "The treasury does not have enough funds." Show the tracked balance. | +| Treasury auth failure | System error: "This withdrawal cannot be completed. Contact an administrator." | + +Gate the UI on `is_member` and the treasury balance before offering the action. + +### `get_proposal(env, proposal_id)` + +| Trigger | Kind | Message | +| ------- | ---- | ------- | +| No proposal with `proposal_id` | State-machine | `proposal not found` | + +Read-only, no authorization. + +--- + +## Atomicity and state on failure + +A panic traps the invocation, and Soroban reverts the **entire transaction**. Therefore: + +- **No storage write survives a failed call.** In `execute_withdraw`, a failure at the treasury withdraw step leaves the proposal `Passed`, not `Executed` — the status update is rolled back with everything else. +- **No event is emitted by a failed call.** Events published before the panic are discarded. A client watching the chain sees nothing at all for a failed invocation — absence of an event is the only signal. +- **Cross-contract effects roll back too.** A failure in `proposals::execute_withdraw` after the treasury transfer would revert the token movement as well. + +The practical consequence for clients: **a failed transaction requires no compensating action.** Re-read state and retry if appropriate. Never attempt to "undo" a failed call. + +--- + +## What a failure looks like from the frontend + +The frontend path is [apps/web/src/lib/soroban.ts](../../apps/web/src/lib/soroban.ts), whose `transferToken` wraps `token_transfer::transfer`. Every failure surfaces as a thrown `Error`, and the **stage at which it throws is the most reliable signal of what went wrong** — far more reliable than the message text. + +The five stages, in order: + +**1. Wallet unavailable** — before any contract interaction: + +```ts +const connectionStatus = await freighter.isConnected(); +if (!connectionStatus.isConnected) { + throw new Error('Freighter not installed or not connected'); +} + +const { address: publicKey, error: addressError } = await freighter.getAddress(); +if (addressError || !publicKey) { + throw new Error('Unable to read Freighter wallet address'); +} +``` + +Not a contract error. Present as a wallet-connection prompt. + +**2. Simulation failure** — where nearly all contract panics surface: + +```ts +const simResult = await server.simulateTransaction(tx); +if (SorobanRpc.Api.isSimulationError(simResult)) { + throw new Error(String(simResult.error)); +} +``` + +**This is the important one.** Simulation executes the contract against current state without submitting, so `amount must be positive`, `not a member`, `already voted`, `proposal is not in Passed state`, and every other deterministic panic is caught here — before the user is ever asked to sign, and before any fee is paid. + +`simResult.error` is the richest diagnostic available anywhere in the flow: it typically contains the host error and, in non-release builds, the panic string. **Log it in full.** Do not show it to users, and do not branch on its text — treat any match as a heuristic for diagnostics only. + +**3. Signature failure** — user rejection: + +```ts +if (signResult.error || !signResult.signedTxXdr) { + throw new Error('Unable to sign transaction with Freighter'); +} +``` + +Almost always the user declining the prompt. Present as a cancellation, not a failure. + +**4. Submission failure**: + +```ts +if (sendResult.status === 'ERROR') { + throw new Error(`Transaction failed: ${String(sendResult.errorResult || sendResult)}`); +} +``` + +Usually a malformed transaction, a bad sequence number, or an insufficient fee — not a contract panic. + +**5. Post-submission revert** — a contract failure that simulation did not predict: + +```ts +if (status.status === SorobanRpc.Api.GetTransactionStatus.FAILED) { + throw new Error(`Transaction reverted: ${hash}`); +} +``` + +Reached when state changed between simulation and inclusion — the classic case being a balance spent by another transaction in the interim, or a proposal finalised by someone else. The message carries only the hash; the reason must be read from the transaction result on-chain. + +**6. Confirmation timeout** — after 30 polls at 2s intervals (~60s): + +```ts +throw new Error(`Transaction not confirmed after timeout: ${hash}`); +``` + +**Not a failure.** The transaction may still succeed. Never present this as an error or invite a retry — a retry risks a double transfer. Show it as pending, keep the hash, and reconcile later. + +### Mapping strategy for `lib/soroban.ts` callers + +Because panic strings are unreliable at the boundary, the durable approach has three parts: + +**Pre-flight in the client.** Every validation failure and most state-machine failures are knowable before submitting. Read `get_proposal`, `is_member`, `balance`, and the vote key, and disable the action with an explanation instead of letting it fail. This is where the specific, useful messages in the per-function tables above belong — not in a catch block. + +**Branch on the stage, not the string.** Simulation failure, signature failure, submission failure, revert, and timeout each warrant a different user-facing treatment regardless of which contract function was called: + +```ts +try { + const hash = await transferToken(recipient, amount, memo); + // success +} catch (err) { + // Distinguish by stage; log err in full for diagnostics. +} +``` + +**Use the invoked function as context.** Since one call site invokes one contract function, the set of possible failures is already narrow. A failed `approve_withdraw` can only be one of six conditions, and pre-flight state tells you which — no message parsing needed. + +**Never show a raw error to a user.** Panic strings, host error codes, and XDR fragments are diagnostic data. Log them; present a mapped message from the tables above. + +For the full invocation flow and network configuration, see [apps/web/docs/api-soroban-client.md](../../apps/web/docs/api-soroban-client.md). + +--- + +## Related documents + +- [Frontend Soroban client](../../apps/web/docs/api-soroban-client.md) — the invocation flow and where each failure surfaces +- [Token transfer contract API](api-token-transfer.md) — full function reference +- [Proposals contract API](api-proposals.md) — full function reference +- [Proposal lifecycle](concepts-proposal-lifecycle.md) — statuses and legal transitions +- [Token transfer flow](concepts-token-transfer-flow.md) — the in-chat payment path +- [Deployment and invocation](api-deployment-invocation.md) — initialization, admin setup, and the treasury-admin wiring `execute_withdraw` depends on +- [Token transfer storage](contracts-token-transfer-storage.md) — storage keys behind the `not initialized` failures +- [Contracts README](../README.md) — workspace layout, toolchain, build and test