From e6e351aca017fb51aa404297957bca8a3928955d Mon Sep 17 00:00:00 2001 From: viettranx Date: Thu, 16 Apr 2026 13:14:17 +0700 Subject: [PATCH] docs: document ACTOR vs SCOPE pattern + #915 changelog entry - agent-identity-conventions.md: new "ActorID vs UserID in Group Chats" section with helper table, group behavior table, propagation chain diagram, group permission policy, legacy-data tolerance notes. - 17-changelog.md: entry covering the security fix, propagation additions, ACTOR migration list, scope-intentional sites, tests, and the no-DB-migration decision with its legacy-fallback rationale. --- docs/17-changelog.md | 45 +++++++++++++ docs/agent-identity-conventions.md | 100 +++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+) diff --git a/docs/17-changelog.md b/docs/17-changelog.md index 25baf84a..34aa4160 100644 --- a/docs/17-changelog.md +++ b/docs/17-changelog.md @@ -4,6 +4,51 @@ All notable changes to GoClaw Gateway are documented here. Format follows [Keep --- +### ACTOR vs SCOPE — #915 group permission fix + propagation (2026-04-16) + +Resolves Issue #915 (Telegram group `write_file` permission denied after `/addwriter`) and closes an adjacent silent-privilege-bypass discovered during the audit. + +#### Security (breaking behavior in group/guild context) + +- **`store.CheckFileWriterPermission` / `CheckCronPermission`** no longer fail-open on empty or synthetic `SenderID` when the context scope is a group/guild. Previous fail-open allowed subagent/delegate/team/dashboard/cron system turns to write files in group chats without a writer grant — silent privilege bypass. Post-fix: empty or synthetic-prefix senders (`subagent:`, `notification:`, `teammate:`, `system:`, `ticker:`, `session_send_tool`) are DENIED in group context. DM / HTTP paths unchanged. +- **`bus.IsInternalSender`** now also recognises `subagent:` prefix (previously missed, causing subagent senders to be treated as real users in permission checks). + +#### Added — propagation of the acting sender through re-ingress + +- `tools.MetaOriginSenderID` metadata key (`origin_sender_id`) — carries the real acting sender through synthetic-sender announce/dispatch paths. +- `tools.AnnounceMetadata.OriginSenderID` field — subagent/delegate announce queue. +- `tools.SubagentTask.OriginSenderID` field — populated at spawn from `store.SenderIDFromContext(ctx)`. +- `cmd.subagentAnnounceRouting.SenderID` — carries sender from handler into the re-ingress `RunRequest`. +- `tools.DelegateRequest.SenderID` field — same propagation for delegate announcements. +- `store.ActorIDFromContext(ctx)` helper — returns `SenderID` if set, else `UserID`. Clarifies ACTOR vs SCOPE at call sites. + +#### Changed — ACTOR migration (call sites now use `ActorIDFromContext`) + +- `internal/tools/publish_skill.go`: skill owner = actor (individual, not group principal). +- `internal/tools/skill_manage.go` (create, patch, delete): owner = actor on create; patch/delete ownership check accepts actor or legacy `UserIDFromContext` for backward compatibility with skills created before this change. +- `internal/tools/delegate_tool.go`: `DelegateRequest.UserID` and `DomainEvent.UserID` = actor for audit trails. +- `internal/tools/team_tasks_blocker.go`: blocker attribution and escalation `UserID` = actor. +- `internal/tools/team_tool_cache.go`: team access-policy check uses actor (fixes group-chat member access where per-user allow/deny lists previously never matched the group principal). +- `internal/tools/team_tool_dispatch.go`: teammate-dispatch metadata now carries `MetaOriginSenderID` so the teammate's turn has the original user's sender. +- `internal/tools/sessions_send.go`: session_send metadata now carries `MetaOriginSenderID` to preserve user identity across agent-to-agent flows. + +#### Scope-intentional (unchanged, commented) + +- `internal/tools/cron.go`: cron jobs remain per-group-scope (memory-model parity; migrating would break collaborative `/cron list/add/remove`). +- `internal/tools/team_tasks_create.go`: team task `UserID` remains per-group (team visibility is per-chat, not per-user). + +#### Tests + +- New `tests/integration/telegram_group_write_file_permission_test.go` — 9 sub-tests covering granted-sender, ungranted-sender, no-agent fail-open branch, DM no-op, `|`-delimited sender, empty-sender deny, synthetic-prefix deny (7 sub-cases), propagated-sender allow, DM empty-sender passes. +- Regression coverage for both the user-reported denial (#915 BUG-B) and the silent-bypass (#915 BUG-A). + +#### Notes + +- No DB migration shipped: historical `skills.owner_id` rows that were created with a `group:*` / `guild:*` value remain accessible via the patch/delete legacy fallback. Re-publishing a skill transfers ownership to the individual actor. +- Audit report: `plans/reports/audit-260416-1240-actor-scope-comprehensive.md`. + +--- + ## [Unreleased] — 2026-04-15 #### Agent Hooks System — Phase 3: Prompt Handler + Web UI (2026-04-15) diff --git a/docs/agent-identity-conventions.md b/docs/agent-identity-conventions.md index 85fe8b37..07b05d68 100644 --- a/docs/agent-identity-conventions.md +++ b/docs/agent-identity-conventions.md @@ -262,4 +262,104 @@ Consequences for code: - 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. +--- + +## ActorID vs UserID in Group Chats + +A second identity pattern complements agent identity: **who is acting** vs +**which scope the action belongs to**. The pattern mirrors agent UUID/key +but applies to end users in group contexts (Telegram, Discord, Feishu, Zalo). + +### The two identities + +| Identity | Context key | Helper | Meaning | +|---|---|---|---| +| **SCOPE** — namespace / memory | `UserIDKey` | `store.UserIDFromContext(ctx)` | `group:telegram:` in Telegram groups; `guild::user:` in Discord guilds; sender ID in DMs | +| **ACTOR** — acting principal | `SenderIDKey` | `store.SenderIDFromContext(ctx)` | Always the individual sender's numeric ID (never group-scoped) | +| (combined) | — | `store.ActorIDFromContext(ctx)` | `SenderID` if set, else falls back to `UserID` | + +### When to use each + +| Purpose | Helper | Why | +|---|---|---| +| Memory / KG / session key | `UserIDFromContext` / `MemoryUserID` / `KGUserID` | Memory is per-group by design — all members share conversational context | +| File / path scope | `UserIDFromContext` | Per-group workspace; group members share files | +| **Permission check** | `SenderIDFromContext` | Checks attribute to the real user, not the group | +| **Audit trail** (`initiated_by`, event `UserID`) | `ActorIDFromContext` | Traces the real user action across wrappers | +| **Ownership** (`OwnerID`, creator fields) | `ActorIDFromContext` | A skill published in a group belongs to the individual user | +| Role / RBAC | `ActorIDFromContext` | Role applies to the human, not the group | + +### Group behavior table + +| Context | `UserID` | `SenderID` | `ActorID` | +|---|---|---|---| +| Telegram DM | sender numeric | sender numeric | sender numeric | +| Telegram group | `group:telegram:` | sender numeric | sender numeric | +| Discord DM | sender snowflake | sender snowflake | sender snowflake | +| Discord guild | `guild::user:` | sender snowflake | sender snowflake | +| HTTP API | HTTP user ID | (empty) | HTTP user ID | +| Cron / subagent system ctx | (inherited / empty) | (empty or propagated) | same as SenderID, else UserID | + +### Propagation through wrappers (#915) + +When a tool wrapper synthesizes an inbound message (subagent announce, +delegate announce, teammate dispatch, session_send), the synthetic +`SenderID` carries routing identity (e.g. `subagent:`) — NOT a +real user. The real acting sender must travel through `InboundMessage.Metadata` +under `tools.MetaOriginSenderID`, and the next-turn builder copies it to +`RunRequest.SenderID`. Without this propagation: + +- Permission checks against `bus.IsInternalSender(...)` prefixes → **denied** + (synthetic senders never match a grant) +- Empty sender → **denied** (`CheckFileWriterPermission` group-context fail-closed + policy) + +Code path: + +``` +SubagentTask.OriginSenderID + → AnnounceMetadata.OriginSenderID + → InboundMessage.Metadata[MetaOriginSenderID] + → subagentAnnounceRouting.SenderID + → RunRequest.SenderID + → loop_context.go:WithSenderID(ctx, …) + → SenderIDFromContext(ctx) in permission checks +``` + +Same chain for delegate (`delegate_tool.announceToParent`) and teammate +dispatch (`team_tool_dispatch.go`). + +### Group permission check policy + +`store.CheckFileWriterPermission` / `store.CheckCronPermission` in group +or guild context (`UserID` starts with `group:` or `guild:`): + +- empty `SenderID` → **DENY** (system context must not gain write access silently) +- synthetic-prefix `SenderID` → **DENY** (`subagent:`, `notification:`, `teammate:`, + `system:`, `ticker:`, `session_send_tool`) +- real numeric `SenderID` → DB lookup; DENY if no `file_writer` allow grant for this sender +- DB error → fail-open (availability over strictness) + +In DM / HTTP / cron-direct context (no group/guild prefix): always allow. +No per-user writer gate applies. + +### Legacy-data tolerance for skill ownership + +Skills / cron / delegate audit trails created before #915 migration store +`UserIDFromContext` values (possibly `group:*` prefixed) in `owner_id` / +`user_id` columns. Ownership checks at `skill_manage.go` (patch/delete) +accept either `ActorIDFromContext` (new) or `UserIDFromContext` (legacy). +This lets existing group-scoped rows remain accessible without a +destructive backfill. When a user re-publishes a skill, the new row uses +actor ownership (tighter by default). + +### Related code + +- `internal/store/context.go` — helper definitions +- `internal/store/config_permission_store.go` — group permission policy +- `internal/tools/subagent_spawn.go`, `subagent_exec.go` — subagent propagation +- `internal/tools/delegate_tool.go` — delegate propagation +- `cmd/gateway_subagent_announce_queue.go`, `gateway_consumer_handlers.go` — re-ingress reconstruction +- `tests/integration/telegram_group_write_file_permission_test.go` — regression fixtures + When in doubt, walk the four-step checklist in section 4.