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.
This commit is contained in:
viettranx committed 2026-04-16 14:17:48 +07:00
1 parent 24a098c580
commit e6e351aca0
2 files changed
+145

No files matched your search

+45
View File
@@ -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)
+100
View File
@@ -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:<chatID>` in Telegram groups; `guild:<guildID>:user:<senderID>` 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:<chatID>` | sender numeric | sender numeric |
| Discord DM | sender snowflake | sender snowflake | sender snowflake |
| Discord guild | `guild:<guildID>:user:<senderID>` | 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:<taskID>`) — 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.