Repository navigation
feat: refactor notification handling to use AccountID and enhance IAM… - #627
Conversation
… principal mapping for members
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This refactors every place that derived an identity from ProjectMember.UserID (nil for non-human members) to use the canonical AccountID, fixing #626 where a service-account assignee was denied by the trigger gate and got no task_assigned conversation.
triggergate/gate.go— newprincipalOf(member)builds aniam.PrincipalfromAccountIDwith the kind fromMemberType; replaces theiam.User(UserID)/ agent-AgentIDbranch.http/middleware/authz_attrs.go—MemberRepoLookup.MemberPrincipalIDnow returnsAccountID(wasAgentIDfor agents,UserIDotherwise), fixingtask.assignee_idfor service-account assignees.service/notification/service.go—NotifyAssignedusesassigneeMember.AccountIDfor the recipient, self-assignment check, and both publish calls, so a service-account assignee no longer inserts a nil-UUID recipient.http/handler/project_handler.go—hasPlatformProjectsReadjudges the request's real principal viamiddleware.IAMPrincipalFrominstead of hardcodingiam.User, soprincipal.typeconditions see the right kind; the unusedaccountIDparameter was dropped.
I traced the load-bearing invariants and found them sound: AccountID equals UserID for humans and AgentID for agents (migration 000066 sets account_id = COALESCE(user_id, agent_id), and projectMemberCols/toMemberEntity populate it), so those two paths are value-identical to before; migration 000068 repointed the notifications.recipient_account_id FK to accounts(id), making an agent/service-account id a valid recipient; accountdom.Kind* and iam.Principal* use the same strings; and the assignment stream's actor_id is the acting account id, so the AccountID == in.ActorID self-assignment check is correct. The NotifyMentioned gap is deliberate and documented.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
…nts and service accounts
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This run re-reviewed the delta since the prior pullfrog review — one new commit that narrows assignment notifications to human assignees, adds a fail-closed guard to the trigger gate, and adds test coverage.
- Bot assignees are no longer notified —
NotifyAssignedreturns early foragent/service_accountmembers, so a non-human assignee no longer inserts a recipient row (or fails the FK on the zero UUID) and can no longer stall the assignment consumer on a failing insert. The agent-run path innotification_consumer.go(TriggerTaskAssigned) is untouched, so thetask_assignedconversation for agent assignees still fires. - Trigger gate fails closed on a missing account —
gate.goreturnserrForbiddenwhenmember.AccountIDis the zero UUID instead of judging a nil-UUID principal. - Test coverage — service-account member grants (allowed/denied) and a nil-account member denied in
triggergate; agent/service-account assignees skipped in the notification service; member-principal lookup keyed onAccountID; andListProjectsjudged against the token's real principal kind.
I traced the load-bearing invariants for the new commit and found them sound: service-account project members are valid accounts rows with account_id set (so the nil-account guard does not reject them), member_type is derived from ac.kind and is never empty, and the task_assigned conversation is produced by the trigger path rather than NotifyAssigned. go test passes for the notification, triggergate, middleware, and handler packages.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Summary
Fixes #626. Assigning a ticket to an agent with a service-account API key moved the ticket to In Progress but never created the
task_assignedconversation, because the trigger gate denied it with "you don't have access to use this agent or its environment".Root cause
Several places derived an IAM principal or recipient from
ProjectMember.UserID, which is only populated for human members (zero-value otherwise). For a service-account member that produced the nil UUID, which matches no grant. Grants are stored and looked up byaccount_idfor every account kind, so the member'sAccountIDis the correct identity.Changes
triggergate/gate.go: newprincipalOf(member)builds the principal fromAccountID, with typeuser/agent/service_accounttaken fromMemberType. This is the bug reported in triggergate denies task_assigned triggers when the assigning actor is a service account #626.middleware/authz_attrs.go:MemberRepoLookup.MemberPrincipalIDreturnsAccountID, sotask.assignee_idis correct for service-account assignees (same value as before for humans and agents).notification/service.go:NotifyAssignedusesAccountIDfor the recipient and the self-assignment check. Previously a service-account assignee produced a nil-UUID recipient and an FK failure on insert.handler/project_handler.go:hasPlatformProjectsReaduses the request's real principal (middleware.IAMPrincipalFrom) instead of hardcodingiam.User, soprincipal.typeconditions see the right kind for service-account and agent tokens. Dropped the now-unusedaccountIDparameter.Tests
triggergate: fixtures now setAccountID; added cases for a service-account member with its own grant (allowed) and without one (denied).notification: the fake member repo defaultsAccountIDtoUserIDfor humans, mirroring the real repository.go test ./internal/...passes.Not changed
NotifyMentionedstill readsUserIDwhen parsing@usernametext, so mentioned agents and service accounts are skipped (nil UUID fails the member lookup). That is harmless and avoids notifying bots unintentionally.