Skip to content

feat: refactor notification handling to use AccountID and enhance IAM… - #627

Merged
pikann merged 2 commits into
masterfrom
feature/enhance-iam-principal-mapping-for-members
Oct 23, 2026
Merged

pikann merged 2 commits into
masterfrom
feature/enhance-iam-principal-mapping-for-members

Conversation

@pikann

@pikann pikann commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

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_assigned conversation, 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 by account_id for every account kind, so the member's AccountID is the correct identity.

Changes

  • triggergate/gate.go: new principalOf(member) builds the principal from AccountID, with type user / agent / service_account taken from MemberType. 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.MemberPrincipalID returns AccountID, so task.assignee_id is correct for service-account assignees (same value as before for humans and agents).
  • notification/service.go: NotifyAssigned uses AccountID for 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: hasPlatformProjectsRead uses the request's real principal (middleware.IAMPrincipalFrom) instead of hardcoding iam.User, so principal.type conditions see the right kind for service-account and agent tokens. Dropped the now-unused accountID parameter.

Tests

  • triggergate: fixtures now set AccountID; added cases for a service-account member with its own grant (allowed) and without one (denied).
  • notification: the fake member repo defaults AccountID to UserID for humans, mirroring the real repository.
  • go test ./internal/... passes.

Not changed

NotifyMentioned still reads UserID when parsing @username text, so mentioned agents and service accounts are skipped (nil UUID fails the member lookup). That is harmless and avoids notifying bots unintentionally.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 — new principalOf(member) builds an iam.Principal from AccountID with the kind from MemberType; replaces the iam.User(UserID) / agent-AgentID branch.
  • http/middleware/authz_attrs.go — MemberRepoLookup.MemberPrincipalID now returns AccountID (was AgentID for agents, UserID otherwise), fixing task.assignee_id for service-account assignees.
  • service/notification/service.go — NotifyAssigned uses assigneeMember.AccountID for 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 — hasPlatformProjectsRead judges the request's real principal via middleware.IAMPrincipalFrom instead of hardcoding iam.User, so principal.type conditions see the right kind; the unused accountID parameter 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.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 — NotifyAssigned returns early for agent/service_account members, 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 in notification_consumer.go (TriggerTaskAssigned) is untouched, so the task_assigned conversation for agent assignees still fires.
  • Trigger gate fails closed on a missing account — gate.go returns errForbidden when member.AccountID is 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 on AccountID; and ListProjects judged 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.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@pikann
pikann merged commit d919813 into master Oct 23, 2026
6 checks passed
@pikann
pikann deleted the feature/enhance-iam-principal-mapping-for-members branch October 23, 2026 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

triggergate denies task_assigned triggers when the assigning actor is a service account

1 participant