From bf771a12f417901ccac5f77afa42e25b67a7c450 Mon Sep 17 00:00:00 2001 From: viettranx Date: Sat, 11 Apr 2026 12:17:59 +0700 Subject: [PATCH] docs(agent-identity): add agent_key vs UUID convention doc Distills the dual-identity rules from the agent identity hardening work into a single permanent reference. Covers all 5 trap zones with post- hardening status, batch fail-fast contract change, router cache canonicalization, FK safety net, telemetry type-layer defense, and dual-tenant agent_key semantics. Adds a CLAUDE.md pointer under Key Patterns so contributors find it via the standard onboarding path. --- CLAUDE.md | 1 + docs/agent-identity-conventions.md | 263 +++++++++++++++++++++++++++++ 2 files changed, 264 insertions(+) create mode 100644 docs/agent-identity-conventions.md diff --git a/CLAUDE.md b/CLAUDE.md index 9595395f..8d891e1e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,6 +74,7 @@ ui/desktop/ Wails v2 desktop app (React frontend + embedded ga - **Store layer:** Interface-based (`store.SessionStore`, `store.AgentStore`, etc.) with shared Dialect pattern in `store/base/`. PostgreSQL (`pg/`) and SQLite (`sqlitestore/`) implementations use `database/sql` + `pgx/v5/stdlib` + sqlx, raw SQL, `BuildMapUpdate()` and `BuildScopeClause()` helpers - **Agent types:** `open` (per-user context, 7 files) vs `predefined` (shared context + USER.md per-user) +- **Agent identity:** Dual-identity pattern (agent_key vs UUID) applies to agents, teams, tenants. Rule: UUID for DB/FK/events, agent_key for logs/paths/UI. See `docs/agent-identity-conventions.md` - **Context files:** `agent_context_files` (agent-level) + `user_context_files` (per-user), routed via `ContextFileInterceptor` - **Providers:** Anthropic (native HTTP+SSE), OpenAI-compat (HTTP+SSE), DashScope (Alibaba Qwen), Claude CLI (stdio+MCP bridge), ACP (Anthropic Console Proxy), Codex (OpenAI). All use `RetryDo()` for retries. Loads from `llm_providers` table with encrypted API keys. ProviderAdapter enables pluggable implementations with ModelRegistry forward-compat resolver. Shared SSEScanner in `providers/sse_reader.go` for streaming providers - **Pipeline:** 8-stage loop (context→history→prompt→think→act→observe→memory→summarize) with pluggable callbacks, always-on execution path diff --git a/docs/agent-identity-conventions.md b/docs/agent-identity-conventions.md new file mode 100644 index 00000000..5ad60092 --- /dev/null +++ b/docs/agent-identity-conventions.md @@ -0,0 +1,263 @@ +# Agent Identity Conventions — `agent_key` vs `UUID` + +> Dual-identity rules for agents, teams, and tenants. Read this before touching any code path that handles identifier strings, DomainEvents, SQL `WHERE agent_id`, router cache, or system prompts. + +## 1. Core rule + +**Question to ask for every identifier:** + +> *Does this identifier ever touch the DB layer or need to be unique system-wide?* +> +> - **Yes** → use `UUID` +> - **No** (display / path / template / log only) → use `agent_key` + +Vietnamese phrasing used by the team: + +> *"Cái identifier này có bao giờ đụng vào DB layer hoặc cần unique toàn hệ thống không? Có → UUID. Không → agent_key."* + +There is no third option. Every site that handles an `AgentID`, `TenantID`, or `TeamID` string must answer this question. + +## 2. Decision table + +### 2.1. MUST use UUID + +| Use case | Reason | Example | +|---|---|---| +| SQL `WHERE`/`JOIN`/`INSERT` on `agent_id` column | DB column type is `uuid` | `episodic_summaries.agent_id`, `kg_entities.agent_id` | +| Store model struct field | Compile-time type safety | `store.EpisodicSummary.AgentID uuid.UUID` | +| `store.WithAgentID(ctx, ...)` | Downstream SQL query context | `loop_context.go` | +| `eventbus.DomainEvent.AgentID` ⚠️ | Consumer workers parse and query DB with it | `episodic_worker`, `semantic_worker`, `dreaming_worker` | +| OpenTelemetry span attribute `agent_id` | Cross-session tracing correlation | `loop_tracing.go` | +| Tenant-scoped FK (team members, agent_links) | DB constraint enforcement | `TeamMember.AgentID`, `AgentLink.SourceAgentID` | +| Public HTTP/WS API `AgentID` params | Stable across renames | `params.AgentID` in heartbeat, config, teams methods | +| `SystemPromptConfig.AgentUUID` | Runtime identification, not exposed to LLM | Paired with `AgentID` (key) | + +### 2.2. MUST use agent_key + +| Use case | Reason | Example | +|---|---|---| +| System prompt template ("You are @X") | LLM/user reads it, must be human-readable | `SystemPromptConfig.AgentID` | +| Filesystem path segment `agents/{X}/...` | Path is user-visible | `workspace.ResolveParams.AgentID` → `sanitizeSegment()` | +| WS `AgentEvent` broadcast to UI | UI filters and displays by name | `AgentEvent.AgentID` | +| Slog log fields (`"agent"` key) | Human-readable when grep'ing logs | `slog.Info(..., "agent", l.id)` | +| `store.WithAgentKey(ctx, ...)` | Intentional key propagation | `loop_context.go` | +| `tools.WithToolAgentKey(ctx, ...)` | Tool access identifier | `loop_context.go` | +| Router cache lookup | Callers have `agent_key` readily available | `router.Get(ctx, agentKey)` | +| Channel bot identifier (Telegram `@X`) | Users type it in chat | Channels layer | +| `Loop.ID() string` method | Display API | `loop_tracing.go` | + +## 3. Trap zones + +Five known identifier trap zones. All are mitigated by the agent identity hardening work (commits `acbfb2e4..d3f37068`). Status per zone below. + +### 🪤 Trap 1: `DomainEvent.AgentID` is a `string` + +- **Problem:** field type is `string`; the compiler cannot enforce UUID vs key. Publishers nearby only see the type, not the semantic intent. If a publisher writes `AgentID: l.id` instead of `l.agentUUID.String()`, downstream consumers (which `uuid.Parse` and query DB) explode with `invalid input syntax for type uuid`. +- **Rule:** if ANY consumer calls `uuid.Parse(event.AgentID)` or passes the value down to SQL `WHERE agent_id = $1`, the publisher MUST set a UUID string. Today every production consumer in `internal/consolidation/*` does exactly that. +- **Status:** Mitigated by the publish-time observer in `internal/eventbus/validate_agent_id.go` — warns with `slog.Warn("eventbus.non_uuid_agent_id", ...)` on any non-UUID drift. Strict type lift to `uuid.UUID` is gated (Phase 5) behind wire-format audit + rolling-deploy coexistence tests. + +### 🪤 Trap 2: Struct with both `AgentID` and `AgentUUID` + +- **Problem:** `SystemPromptConfig` has `AgentID string` (agent_key) + `AgentUUID string` (UUID). Two adjacent `string` fields are trivially swappable at a callsite. +- **Rule:** always check field comments or downstream consumer behavior before setting either field. `AgentID` is rendered into the system prompt ("You are @{AgentID}"); `AgentUUID` is for runtime identification and never reaches the LLM. +- **Status:** Mitigated. The one historical omission (`memoryflush.go` missing `AgentUUID`) is now set. See section 10 for why the omission worked by accident historically. + +### 🪤 Trap 3: `mustParseUUID` silently returns `uuid.Nil` + +- **Problem:** a store helper named `mustParseUUID` looked like `uuid.MustParse` from the stdlib (which panics), but actually behaved like `uuid.Parse` with the error swallowed — returning `uuid.Nil` on bad input. 88 callsites in `internal/store/pg/`. Passing an `agent_key` in would silently write `agent_id = '00000000-0000-0000-0000-000000000000'` or, for reads, return an empty result / zero-row update. +- **Rule:** two helpers, two contracts. + - `parseUUID(s string) (uuid.UUID, error)` — use for **every** `INSERT`/`UPDATE`/`UPSERT`/`DELETE` and any `SELECT WHERE` where an empty result would hide a bug. Propagate the error. Callers fail fast with a clean Go error instead of a cryptic PG 23503 or a silently corrupted row. + - `parseUUIDOrNil(s string) uuid.UUID` — honest rename of the former `mustParseUUID`. Intentionally silent. Only acceptable on read-only `SELECT WHERE` paths where a no-match on bad input is the correct semantics. Prefer `parseUUID` for any new code. +- **Status:** Mitigated by the `parseUUID` refactor across `internal/store/pg/`. 57 critical write sites migrated. The remaining `parseUUIDOrNil` sites are audited and intentional. + +### 🪤 Trap 4: `Router.Get(ctx, X)` accepts both UUID and agent_key + +- **Problem:** the router resolver parses either a UUID or an agent_key and returns the same `Loop`. Pre-hardening, the cache key was the raw input string — so the same agent could end up with two cache entries (one under UUID, one under agent_key), defeating invalidation. In addition, `HasSuffix`-based invalidation could collide with unrelated keys that happened to share a suffix. +- **Rule:** cache entries are **canonicalized** to `tenantID:agentKey` after resolution, regardless of whether the caller passed UUID or key. Invalidation uses exact-segment match, not substring. `IsRunning(ctx, agentKey)` is tenant-aware and requires `tenantID` from context. +- **Status:** Mitigated. See section 8 for the full cache strategy. + +### 🪤 Trap 5: WS method params `AgentID` — inconsistent parsing + +- **Problem:** WS handlers variously used bare `uuid.Parse(params.AgentID)`, `resolveAgentUUID(...)`, or passed straight to the router. From the client's perspective there was no single rule for whether to send UUID or agent_key. +- **Rule:** WS handlers accept **both** UUID and agent_key for `AgentID` params and resolve through a single helper. Hot-path handlers use `resolveAgentUUIDCached` which consults the router cache first and falls back to a DB lookup on miss. Cold-path handlers use `resolveAgentUUID` directly. +- **Status:** Mitigated. Standardized through `internal/gateway/methods/agent_links.go` helpers. + +## 4. Code review sanity checklist + +For every new `AgentID:` assignment or new `agent_id` read/write, walk through in order: + +### Step 1 — Destination field type? + +| Destination type | Required source | +|---|---| +| `uuid.UUID` | UUID source only: `l.agentUUID`, `ag.ID`, `store.AgentIDFromContext(ctx)` | +| `*uuid.UUID` | Same as above, nullable | +| `string` | → Step 2 | + +### Step 2 — If destination is `string`, what does the consumer do with it? + +| Consumer behavior | Required identifier | +|---|---| +| `uuid.Parse(...)` or passes to SQL `WHERE x = $1` on a uuid column | UUID string | +| Logs / displays / compares / renders template / builds path | `agent_key` string | +| Forwards to another consumer | Follow the terminal consumer | + +### Step 3 — Double-check with grep + +```bash +grep -rn "\.AgentID" +``` + +Look for any `uuid.Parse(...)` or SQL `WHERE agent_id = $1` in the downstream path. If any consumer eventually touches DB, the publisher must supply a UUID. + +### Step 4 — Run the regression tests + +```bash +go test ./internal/consolidation/... ./internal/eventbus/... ./internal/store/pg/... +``` + +Relevant tests: + +- `consolidation/workers_test.go::TestEpisodicWorkerHandle_NonUUIDAgentID` — rejects `"goctech-leader"` with a clean error. +- `consolidation/workers_test.go::TestEpisodicWorkerHandle_NonUUIDTenantID` — mirror for tenant. +- `eventbus/validate_agent_id_test.go` — publish-time observer coverage. +- `store/pg/parseuuid_test.go` — helper contract. +- `agent/router_cache_canonicalize_test.go`, `router_invalidate_collision_test.go`, `router_isrunning_test.go` — router cache strategy. +- `gateway/methods/agent_resolve_cached_test.go` — cache-aware resolver. + +## 5. How this applies to teams and tenants + +The dual-identity pattern is a **codebase-wide convention**, not agent-specific: + +| Entity | UUID field | Key field | Notes | +|---|---|---|---| +| Agent | `agents.id uuid` | `agents.agent_key text` | Dual-tenant: same key across tenants is allowed | +| Team | `agent_teams.id uuid` | `agent_teams.team_key text` | Team members reference agents by UUID | +| Tenant | `tenants.id uuid` | `tenants.tenant_slug text` | Slug used in URLs and onboarding wizard | +| Session | `sessions.session_key text` | (no separate UUID) | Session key is the only identifier — safe by construction | + +The rules in section 2 apply verbatim to `team_id`/`team_key` and `tenant_id`/`tenant_slug`. In particular: + +- `WithTenantID(ctx, ...)` takes a `uuid.UUID`. Never pass a slug. +- `TeamMember.AgentID` is the canonical team-membership FK and must be a UUID. +- Slog log fields `"tenant"`, `"team"` should receive the human-readable slug/key, not the UUID. + +## 6. Post-hardening status + +| Trap zone | Status | Guardrail | +|---|---|---| +| Trap 1 — `DomainEvent.AgentID` as string | Mitigated (strict type gated) | `eventbus/validate_agent_id.go` publish observer | +| Trap 2 — `SystemPromptConfig` dual field | Mitigated | `memoryflush.go` + `loop_history.go` set both fields | +| Trap 3 — `mustParseUUID` silent nil | Mitigated | `parseUUID` error-propagating helper in 57 critical sites | +| Trap 4 — Router cache fragmentation | Mitigated | Canonical `tenantID:agentKey` keys + exact-segment invalidation | +| Trap 5 — WS method inconsistency | Mitigated | `resolveAgentUUID` / `resolveAgentUUIDCached` helpers | + +Phase 5 (DomainEvent strict-type lift) is **gated** — only starts once Phases 1-4 have been stable on `dev` for ≥72h with zero `eventbus.non_uuid_agent_id` observer warnings, and after a coexistence test covering rolling deploys. Without those signals, the runtime observer is sufficient defense. + +## 7. Regression test coverage map + +| Test | File | Guards | +|---|---|---| +| `TestEpisodicWorkerHandle_NonUUIDAgentID` | `internal/consolidation/workers_test.go` | Trap 1 — consumer fails cleanly on bad `AgentID` | +| `TestEpisodicWorkerHandle_NonUUIDTenantID` | `internal/consolidation/workers_test.go` | Trap 1 — same for tenant | +| `TestValidateAgentID_*` | `internal/eventbus/validate_agent_id_test.go` | Trap 1 — publish-time observer warns on drift | +| `TestParseUUID*` / `TestParseUUIDOrNil*` | `internal/store/pg/parseuuid_test.go` | Trap 3 — helper contract (error vs silent) | +| `TestRouterGet_Canonicalize*` | `internal/agent/router_cache_canonicalize_test.go` | Trap 4 — UUID and key both map to same canonical cache entry | +| `TestRouterInvalidate_SuffixCollision` | `internal/agent/router_invalidate_collision_test.go` | Trap 4 — exact-segment invalidation does not collide on substring | +| `TestRouterIsRunning_*` | `internal/agent/router_isrunning_test.go` | Trap 4 — `IsRunning` is tenant-scoped, not tenant-blind | +| `TestRouterGet_DualTenantSameAgentKey` | `internal/agent/router_cache_canonicalize_test.go` | Dual-tenant agent_key isolation (see section 13) | +| `TestAgentResolveCached_*` | `internal/gateway/methods/agent_resolve_cached_test.go` | Trap 5 — cache-aware fast path plus DB fallback | +| `TestVaultHandlerUpload_*` | `internal/http/vault_handler_upload_test.go` | HTTP form `agent_id` entry point (bypass gap for owner / lite edition) | +| `TestMemoryFlush_*` | `internal/agent/memoryflush_test.go` | Trap 2 — `SystemPromptConfig.AgentUUID` is set | + +## 8. Router cache canonicalization strategy + +Cache invariants after hardening: + +1. **Key format:** `{tenantID}:{agentKey}`. Computed AFTER the resolver returns an `Agent`. Inputs that arrived as UUID are translated to `agentKey` via the resolved record. No more dual entries. +2. **Tenant scoping:** all router operations take `ctx` and extract tenant via `store.TenantIDFromContext`. `Router.Get`, `Router.GetCached`, `Router.IsRunning`, and `InvalidateAgent` all require a tenant. +3. **Invalidation:** exact-segment match on the full canonical key. The previous `strings.HasSuffix(entry, agentKey)` approach could collide when one agent's key was a suffix of another's. Now replaced with exact match against `{tenantID}:{agentKey}`. +4. **`IsRunning(ctx, agentKey)`:** tenant-scoped. Pre-hardening, the check was tenant-blind and always returned `false` for any tenant whose agent_key collided with another tenant's loop (see section 13). + +## 9. Batch fail-fast contract + +Pre-hardening: store methods that accepted a batch (vault bulk upload, KG ingest, memory doc batch) called `mustParseUUID` on each row. Bad UUIDs resolved to `uuid.Nil`, the `INSERT` went through, and the DB rejected it via FK code 23503 — **for the first bad row only**. Successfully inserted rows stayed. Callers saw a partial success. + +Post-hardening: `parseUUID` errors at parse time. The entire batch fails fast at the first invalid UUID, **before any write happens**. Net contract change: + +- Error is raised earlier, not silent-to-loud. +- Callers that relied on partial success must validate UUIDs up front or split the batch themselves. +- Clients using bulk operations cannot mix valid and invalid UUIDs in the same call. + +**Affected operations:** vault bulk upload, knowledge graph batch ingest, memory documents batch upsert. + +This is an intentional contract change and is documented here rather than hidden in a release note, because callers outside this repo (if any) will see the new error type. + +## 10. memoryflush historical accident + +`memoryflush.go` historically set only `SystemPromptConfig.AgentID` and not `AgentUUID`. This did not cause a bug because `buildRuntimeSection` — the function that consumes `AgentUUID` — runs **after** the `CacheBoundaryMarker` in the assembled system prompt. Anything below the marker is rendered fresh every turn and never feeds into the stable prefix that gets cached by the LLM provider. Missing `AgentUUID` simply meant an empty runtime field in the dynamic suffix. No prompt cache bust, no functional consequence. + +The field is now populated unconditionally for consistency with `loop_history.go`, which has always set both fields. Mentioned here because anyone reading the fix is likely to wonder what broke — nothing broke, it was a latent gap that would have become a real bug if `AgentUUID` ever moved above the cache boundary. + +## 11. FK as safety net + +PostgreSQL and SQLite both enforce FK constraints on `agent_id` → `agents.id`. This is the **primary correctness guarantee** against silent data corruption, not the `parseUUID` refactor. + +**PostgreSQL:** all primary phase 4 target tables have an FK with `CASCADE` (or `SET NULL` for nullable columns). Verified against staging on 2026-04-11 across `episodic_summaries`, `kg_entities`, `kg_relations`, `memory_documents`, `memory_chunks`, `vault_documents` — plus 25 other agent-scoped tables. Bad UUIDs are rejected with PG error code 23503. + +**SQLite:** `PRAGMA foreign_keys = ON` is applied per-connection via `sqlitestore/pool.go` and `schema.sql`. All `agent_id TEXT` columns declare `REFERENCES agents(id) ON DELETE CASCADE`. Bad input → SQLite constraint error. + +**Orphan scan:** staging MCP scan on 2026-04-11 found **zero** rows with `agent_id = '00000000-0000-0000-0000-000000000000'` across 5 NOT-NULL target tables. Self-hosted deployments that predate the FK migrations can run `scripts/scan_orphan_agent_rows.sh` as a pre-flight check before upgrading. + +**What the `parseUUID` refactor adds on top of FK:** + +- Earlier error detection (before the SQL round-trip). +- A clean Go error with a descriptive message instead of a driver-layer PG 23503. +- Coverage for `UPDATE WHERE agent_id = uuid.Nil` and `SELECT WHERE agent_id = uuid.Nil` — FK cannot help on reads or no-op updates. This is where the real silent-bug risk lived post-FK, and the refactor closes it. + +**If a future developer misses a `parseUUID` site:** the result is a cryptic PG error at the driver layer, not corruption. The FK safety net still holds. + +## 12. Telemetry tables without FK + +`spans`, `traces`, `usage_snapshots` do **not** have FK on `agent_id`. Reason: these are append-only, high-volume telemetry tables — FK validation overhead at insert rate would be prohibitive. + +**Type-layer defense instead of FK:** + +- All struct `AgentID` fields in `store/pg/tracing.go`, `store/pg/snapshot.go`, etc. are `*uuid.UUID`, not `string`. +- Inserts go through `store/base/helpers.go::nilUUID()` which converts `uuid.Nil → SQL NULL`. +- There is no `mustParseUUID` call site in telemetry store code — callers cannot pass bare strings. The compile-time contract blocks the trap entirely. + +**Residual risk:** if a caller bypasses `nilUUID()` and writes `uuid.Nil` raw, the row lands with `agent_id = '00000000-0000-0000-0000-000000000000'`. Impact is scoped to analytics pollution (these tables feed dashboards, not downstream pipelines). Check with: + +```sql +SELECT count(*) FROM spans WHERE agent_id = '00000000-0000-0000-0000-000000000000'::uuid; +``` + +If the count ever rises above zero on production, it is a caller bug in the telemetry emission path — not a store-layer regression. + +## 13. Dual-tenant `agent_key` + +`agent_key` is unique **only within a tenant**. The same `agent_key` can exist in multiple tenants with different UUIDs. Example from staging (2026-04-11): `tieu-ho` exists in both the Master tenant and the Việt Org tenant under different `agents.id` values. + +The unique index is: + +```sql +CREATE UNIQUE INDEX agents_tenant_key_unique + ON agents (tenant_id, agent_key) + WHERE deleted_at IS NULL; +``` + +Consequences for code: + +- **Never** use bare `agent_key` as a global identifier. It is only unique inside a tenant. +- `Router.Get(ctx, "tieu-ho")` requires `tenantID` from ctx. Without tenant scoping, the lookup is ambiguous across tenants. +- Cache entries are keyed as `{tenantID}:{agentKey}` (section 8). +- Log formatting: prefer `{"agent": "tieu-ho", "tenant": "master"}` together, or just the UUID. Never log `agent_key` alone without a tenant. +- When designing new tables with agent_key columns, the unique index must include `tenant_id`. Scoping to tenant_id prevents the same error class in future features. + +## Appendix — TL;DR one-liner + +- Use **`l.agentUUID.String()`** when setting `DomainEvent.AgentID`, store model fields, SQL query parameters, OTel span attributes, FK constraints, and any tenant-scoped unique key. +- Use **`l.id`** when logging, emitting `AgentEvent` for the UI, rendering the system prompt, building filesystem paths, setting the key context (`WithAgentKey`), and doing router lookups. + +When in doubt, walk the four-step checklist in section 4.