feat: Microsoft Teams integration (calls/room, calls/user, meetings)#316
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds three NATS RPC handlers to ChangesMicrosoft Teams Integration: Calls & Meetings RPCs
Sequence Diagram(s)sequenceDiagram
participant Client
participant roomService as room-service Handler
participant TeamsMeetingStore as MongoDB TeamsMeetingStore
participant graphClient as pkg/msgraph Client
participant MsgStream as Canonical Message Stream
Client->>roomService: NATS RPC — TeamsRoomCall / TeamsUserCall
roomService->>roomService: validate account/roomID, check membership, build deep-link URL
roomService-->>Client: TeamsCallReply { joinUrl }
Client->>roomService: NATS RPC — TeamsMeeting
roomService->>roomService: validate config, membership, member count
roomService->>TeamsMeetingStore: GetTeamsMeeting(roomID, siteID)
alt record found (fast-path)
TeamsMeetingStore-->>roomService: TeamsMeetingRecord
roomService-->>Client: TeamsMeetingReply { id, joinUrl }
else no record
TeamsMeetingStore-->>roomService: (nil, false, nil)
roomService->>graphClient: CreateOnlineMeeting(externalId=siteID:roomID)
graphClient-->>roomService: OnlineMeeting { id, joinWebUrl }
roomService->>TeamsMeetingStore: InsertTeamsMeeting(record)
alt duplicate-key race (concurrent writer won)
TeamsMeetingStore-->>roomService: duplicate-key error
roomService->>TeamsMeetingStore: GetTeamsMeeting(roomID, siteID)
TeamsMeetingStore-->>roomService: winner record
else this writer is the winner
roomService->>MsgStream: publish teams_meet_started (dedup ID)
end
roomService-->>Client: TeamsMeetingReply { id, joinUrl }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/subject/subject.go (1)
754-756: ⚡ Quick winValidate
accountin the new Teams subject builders for wildcard safety.
TeamsRoomCall,TeamsMeeting, andTeamsUserCallacceptaccountbut skip the shared token guard used elsewhere in this file. AddisValidAccountToken(account)checks for consistent NATS-subject safety.Suggested patch
func TeamsRoomCall(account, roomID, siteID string) string { + if !isValidAccountToken(account) { + panic("invalid account token: contains NATS wildcard characters") + } return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.call", account, roomID, siteID) } ... func TeamsMeeting(account, roomID, siteID string) string { + if !isValidAccountToken(account) { + panic("invalid account token: contains NATS wildcard characters") + } return fmt.Sprintf("chat.user.%s.request.room.%s.%s.teams.meeting", account, roomID, siteID) } ... func TeamsUserCall(account, siteID string) string { + if !isValidAccountToken(account) { + panic("invalid account token: contains NATS wildcard characters") + } return fmt.Sprintf("chat.user.%s.request.teams.%s.call.user", account, siteID) }Based on learnings: for
pkg/subject, functions accepting account tokens should consistently reject wildcard tokens via a shared validator.Also applies to: 764-766, 774-776
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/subject/subject.go` around lines 754 - 756, The functions TeamsRoomCall, TeamsMeeting, and TeamsUserCall accept an account parameter but lack validation checks for wildcard safety, which is inconsistent with other subject builders in the file that use isValidAccountToken(account). Add isValidAccountToken(account) checks at the beginning of each of these three functions to validate the account parameter and return an empty string if validation fails, ensuring consistent NATS-subject safety across all account-accepting subject builders.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/client-api.md`:
- Around line 1891-1894: The documentation for the triggered events success path
currently describes the `teams_meet_started` system message publication without
clarifying that this only occurs on initial meeting creation. Modify the text in
the "Triggered events — success path" section to explicitly state that the
`teams_meet_started` system message is emitted only when a new meeting is first
created, and clarify that idempotent repeat calls which return cached meeting
data do not trigger a new system message emission. This distinction is important
for understanding the behavior difference between first-time and cached
responses.
In `@pkg/msgraph/msgraph.go`:
- Around line 23-27: Rename the single-method `Client` interface to follow the
"-er" suffix convention used for single-method interfaces (e.g.,
`OnlineMeetingCreator` or similar name ending in "-er" that describes the
action). Update the interface definition and its method `CreateOnlineMeeting` to
use the new name. Update the constructor (likely the `New` function around line
92-106) to return the concrete struct type instead of the interface, and update
all usages of the `Client` interface type to reference the struct type instead,
aligning with the "accept interfaces, return structs" convention.
- Around line 227-229: The error message in the createOnlineMeeting function
(around the status code validation) is exposing the raw Graph response body by
including string(respBody) in the fmt.Errorf call. Remove the string(respBody)
portion from the error return statement and only include the status code in the
message. This prevents sensitive information from the upstream Graph API
response from being propagated into error logs and ensures internal error
details are not exposed to callers.
- Around line 8-19: The code uses direct net/http client calls for OAuth2 token
endpoint requests and Graph API calls, which violates the repository standard
requiring Resty for all outbound HTTP calls. Replace the direct net/http client
usage in both locations (the OAuth2 token endpoint call and the Graph API call)
with pkg/restyutil.New() to enable automatic OTel instrumentation and structured
logging. If testing requires a custom HTTP client, use the WithHTTPClient option
provided by restyutil instead of passing the raw http.Client directly.
In `@room-service/handler_teams.go`:
- Around line 116-124: The room-level idempotency check in the handler_teams.go
file has a race condition between reading the marker with
GetLastTeamsMeetStarted and performing the CreateOnlineMeeting operation.
Concurrent calls or fast retries can all see no existing marker and proceed to
create duplicate meetings before the marker is durably written. To fix this,
ensure the marker write happens atomically with or before the
CreateOnlineMeeting call. Consider either creating the meeting first and then
durably writing the marker, or implementing a mutual exclusion mechanism (such
as a distributed lock or database constraint) to guarantee only one concurrent
caller can proceed through the CreateOnlineMeeting step. This ensures that by
the time CreateOnlineMeeting is invoked, the marker is already durably visible
to prevent subsequent concurrent calls from making duplicate create requests.
In `@room-service/main.go`:
- Around line 49-62: Add startup validation checks to fail fast on invalid Teams
configuration before the service starts accepting requests. Validate that if any
of the Teams credentials (TeamsTenantID, TeamsClientID, TeamsClientSecret) are
set, all three must be provided together to avoid partial configuration; also
validate that TeamsEmailDomain is not empty and that RoomMembersLimit and
RoomMembersCallLimit are positive integers. If any validation fails, log a
descriptive error message and exit with a non-zero status code, following the
pattern established for bucket settings validation. This validation should
happen during startup initialization to prevent runtime failures in Teams RPCs.
---
Nitpick comments:
In `@pkg/subject/subject.go`:
- Around line 754-756: The functions TeamsRoomCall, TeamsMeeting, and
TeamsUserCall accept an account parameter but lack validation checks for
wildcard safety, which is inconsistent with other subject builders in the file
that use isValidAccountToken(account). Add isValidAccountToken(account) checks
at the beginning of each of these three functions to validate the account
parameter and return an empty string if validation fails, ensuring consistent
NATS-subject safety across all account-accepting subject builders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ff146dd-3fd5-4e0f-95a8-4a224cda3ee7
📒 Files selected for processing (16)
docs/client-api.mdpkg/model/event.gopkg/model/message.gopkg/model/teams.gopkg/msgraph/msgraph.gopkg/msgraph/msgraph_test.gopkg/subject/subject.goroom-service/deploy/docker-compose.ymlroom-service/handler.goroom-service/handler_teams.goroom-service/handler_teams_test.goroom-service/helper.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_cassandra.go
Comment on lines
+8
to
+19
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "net/url" | ||
| "strings" | ||
| "sync" | ||
| "time" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "msgraph.go" | head -20Repository: hmchangw/chat
Length of output: 82
🏁 Script executed:
wc -l pkg/msgraph/msgraph.goRepository: hmchangw/chat
Length of output: 84
🏁 Script executed:
cat -n pkg/msgraph/msgraph.go | sed -n '1,50p'Repository: hmchangw/chat
Length of output: 2054
🏁 Script executed:
cat -n pkg/msgraph/msgraph.go | sed -n '85,150p'Repository: hmchangw/chat
Length of output: 2467
🏁 Script executed:
cat -n pkg/msgraph/msgraph.go | sed -n '200,239p'Repository: hmchangw/chat
Length of output: 1689
🏁 Script executed:
rg "github.com/go-resty/resty" --type go | head -20Repository: hmchangw/chat
Length of output: 357
🏁 Script executed:
rg "resty\." --type go | head -30Repository: hmchangw/chat
Length of output: 1241
🏁 Script executed:
grep -r "http.Client" --include="*.go" | head -20Repository: hmchangw/chat
Length of output: 672
🏁 Script executed:
cat -n pkg/msgraph/msgraph.go | sed -n '50,85p'Repository: hmchangw/chat
Length of output: 1324
🏁 Script executed:
cat -n pkg/restyutil/restyutil.go | head -50Repository: hmchangw/chat
Length of output: 1714
🏁 Script executed:
rg "msgraph\." --type go | head -20Repository: hmchangw/chat
Length of output: 1130
🏁 Script executed:
grep -A 5 "import" pkg/msgraph/msgraph.go | head -15Repository: hmchangw/chat
Length of output: 116
Replace direct net/http calls with Resty for outbound Graph and token requests.
This implementation uses net/http directly for both the OAuth2 token endpoint (lines 131–137) and Graph API calls (lines 210–217), which diverges from the repository's outbound HTTP standard.
Per coding guidelines: "Use Resty for all outbound HTTP calls — never net/http client directly." Migrate to pkg/restyutil.New() to enable automatic OTel instrumentation and structured logging for both calls. The WithHTTPClient option can be preserved for testing without needing the raw http.Client.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/msgraph/msgraph.go` around lines 8 - 19, The code uses direct net/http
client calls for OAuth2 token endpoint requests and Graph API calls, which
violates the repository standard requiring Resty for all outbound HTTP calls.
Replace the direct net/http client usage in both locations (the OAuth2 token
endpoint call and the Graph API call) with pkg/restyutil.New() to enable
automatic OTel instrumentation and structured logging. If testing requires a
custom HTTP client, use the WithHTTPClient option provided by restyutil instead
of passing the raw http.Client directly.
Source: Coding guidelines
Collaborator
Author
There was a problem hiding this comment.
Leaving this as net/http for now — out of the agreed quick-win scope for this round. Happy to migrate to pkg/restyutil in a follow-up if you'd prefer the OTel/logging consistency; flagging it as a deliberate deferral rather than an oversight.
There was a problem hiding this comment.
@chenjr0719, understood — noted as a deliberate deferral rather than an oversight. The concern remains valid for consistency with the codebase's OTel/logging standard, so it's worth tracking. Would you like me to open a GitHub follow-up issue to capture the pkg/msgraph → pkg/restyutil migration so it doesn't get lost between rounds?
(˳˘ ɜ˘)˳ ♡
🧠 Learnings used
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/roomkeysender/roomkeysender.go:28-30
Timestamp: 2026-04-08T08:25:32.108Z
Learning: In this repository’s Go code, follow the CLAUDE.md rule: don’t add defensive error handling, fallbacks, or nil/validation guards for conditions that are guaranteed not to occur. For internal functions/methods (e.g., when all callers are known to pass non-nil pointer parameters, such as pkg/roomkeysender/roomkeysender.go’s Send), the absence of nil-pointer guards is acceptable—do not flag it as a review issue. Reserve pointer validation and other safety checks for system boundaries (e.g., external inputs, API/HTTP requests, deserialization, or other untrusted entry points).
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/model/model_test.go:306-317
Timestamp: 2026-04-08T08:25:48.132Z
Learning: In Go, `time.Time` is `comparable` and therefore may be used as a type argument to generic constraints requiring `comparable` (e.g., `func roundTrip[T comparable](...)`). This is because `time.Time`’s underlying fields are all comparable (`uint64`, `int64`, and a pointer `*Location`, where pointers are comparable by address). As a result, any struct that contains or embeds a `time.Time` field (e.g., `model.Message`, or structs embedding it like `model.NotificationEvent`) should also satisfy `comparable` and should not be flagged as a compilation-error when used with `T comparable`.
Learnt from: general-lex
Repo: hmchangw/chat PR: 141
File: message-worker/main.go:95-99
Timestamp: 2026-05-04T03:30:38.678Z
Learning: In hmchangw/chat, `pkg/natsutil/connect.go`’s `natsutil.Connect` returns an OpenTelemetry-wrapped NATS connection (`*otelnats.Conn` from `github.com/Marz32onE/instrumentation-go/otel-nats/otelnats`), not a plain `*nats.Conn`. Because `otelnats.Conn.Publish` is defined as `Publish(ctx context.Context, subj string, data []byte) error` (it uses `ctx` to propagate traces via NATS headers), code review should NOT flag `nc.Publish(ctx, subj, data)` as an incorrect method call shape. This `Publish(ctx, ...)` pattern is the standard/correct usage across the codebase (e.g., message-worker, broadcast-worker, notification-worker, history-service).
Learnt from: Allan-Code-hub
Repo: hmchangw/chat PR: 263
File: upload-service/main.go:111-115
Timestamp: 2026-06-03T03:26:01.642Z
Learning: In this repo’s Go code, `mongoutil.Disconnect(ctx context.Context, client *mongo.Client)` returns no value and internally logs disconnect errors (e.g., via `slog.Error("mongo disconnect failed", ...)`). When reviewing, never suggest `return mongoutil.Disconnect(...)`—it will not compile. Instead, call it as a statement (`mongoutil.Disconnect(ctx, client)`) and then return `nil` (or the appropriate value for the surrounding function) if needed. This call pattern should be consistent across services’ shutdown paths (e.g., `*/main.go`).
Comment on lines
+49
to
+62
| // Microsoft Teams integration. Teams* credentials are required only for the | ||
| // meetings RPC (Graph onlineMeeting create); the deep-link RPCs use only | ||
| // EmailDomain. When TenantID/ClientID/ClientSecret are unset the meetings RPC | ||
| // returns errTeamsNotConfigured; the deep-link RPCs still work. | ||
| TeamsTenantID string `env:"TEAMS_TENANT_ID" envDefault:""` | ||
| TeamsClientID string `env:"TEAMS_CLIENT_ID" envDefault:""` | ||
| TeamsClientSecret string `env:"TEAMS_CLIENT_SECRET" envDefault:""` | ||
| TeamsEmailDomain string `env:"TEAMS_EMAIL_DOMAIN" envDefault:"dev.local"` | ||
| RoomMembersLimit int `env:"ROOM_MEMBERS_LIMIT" envDefault:"500"` | ||
| RoomMembersCallLimit int `env:"ROOM_MEMBERS_CALL_LIMIT" envDefault:"20"` | ||
| // MessageBucketHours must match message-worker's MESSAGE_BUCKET_HOURS so the | ||
| // teams_meet_started idempotency read walks the same partitions writes land in. | ||
| MessageBucketHours int `env:"MESSAGE_BUCKET_HOURS" envDefault:"72"` | ||
| MessageReadMaxBuckets int `env:"MESSAGE_READ_MAX_BUCKETS" envDefault:"30"` |
There was a problem hiding this comment.
Fail fast on invalid/partial Teams startup config instead of deferring to runtime errors.
The new startup checks validate bucket settings, but invalid Teams runtime config can still slip through (non-positive member limits, empty email domain, or partially set Graph credentials). That leads to hard-to-diagnose runtime behavior where Teams RPCs fail or are silently disabled.
Suggested patch
import (
"context"
"fmt"
"log/slog"
"net/url"
"os"
+ "strings"
"time"
@@
if cfg.MessageReadMaxBuckets <= 0 {
slog.Error("invalid MESSAGE_READ_MAX_BUCKETS: must be > 0", "value", cfg.MessageReadMaxBuckets)
os.Exit(1)
}
+ if cfg.RoomMembersLimit <= 0 {
+ slog.Error("invalid ROOM_MEMBERS_LIMIT: must be > 0", "value", cfg.RoomMembersLimit)
+ os.Exit(1)
+ }
+ if cfg.RoomMembersCallLimit <= 0 {
+ slog.Error("invalid ROOM_MEMBERS_CALL_LIMIT: must be > 0", "value", cfg.RoomMembersCallLimit)
+ os.Exit(1)
+ }
+ if strings.TrimSpace(cfg.TeamsEmailDomain) == "" {
+ slog.Error("invalid TEAMS_EMAIL_DOMAIN: must be non-empty")
+ os.Exit(1)
+ }
@@
var graphClient msgraph.Client
- if cfg.TeamsTenantID != "" && cfg.TeamsClientID != "" && cfg.TeamsClientSecret != "" {
+ hasAnyTeamsCred := cfg.TeamsTenantID != "" || cfg.TeamsClientID != "" || cfg.TeamsClientSecret != ""
+ hasAllTeamsCred := cfg.TeamsTenantID != "" && cfg.TeamsClientID != "" && cfg.TeamsClientSecret != ""
+ if hasAnyTeamsCred && !hasAllTeamsCred {
+ slog.Error("invalid Teams Graph config: TEAMS_TENANT_ID, TEAMS_CLIENT_ID, and TEAMS_CLIENT_SECRET must be set together")
+ os.Exit(1)
+ }
+ if hasAllTeamsCred {
graphClient = msgraph.New(msgraph.Config{
TenantID: cfg.TeamsTenantID,
ClientID: cfg.TeamsClientID,
ClientSecret: cfg.TeamsClientSecret,
})
}As per coding guidelines: **/*main*.go requires services to fail fast on invalid startup config (log error + non-zero exit).
Also applies to: 156-163, 174-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@room-service/main.go` around lines 49 - 62, Add startup validation checks to
fail fast on invalid Teams configuration before the service starts accepting
requests. Validate that if any of the Teams credentials (TeamsTenantID,
TeamsClientID, TeamsClientSecret) are set, all three must be provided together
to avoid partial configuration; also validate that TeamsEmailDomain is not empty
and that RoomMembersLimit and RoomMembersCallLimit are positive integers. If any
validation fails, log a descriptive error message and exit with a non-zero
status code, following the pattern established for bucket settings validation.
This validation should happen during startup initialization to prevent runtime
failures in Teams RPCs.
Source: Coding guidelines
Collaborator
Author
There was a problem hiding this comment.
Good catch — out of scope for this quick-win round, but I agree the partial-Teams-creds case should fail fast. Flagging as a follow-up; the current behavior is: all-three-creds set → meetings enabled, none set → meetings report not-configured, partial → currently constructs no client (meetings disabled) which is the silent case you note.
There was a problem hiding this comment.
@chenjr0719, understood — the current silent-disable behavior for partial credentials is intentional and self-consistent within this PR. Would you like me to open a GitHub follow-up issue to track the three startup guards:
- Partial Teams credentials (any-but-not-all) → fail fast with a descriptive error
TEAMS_EMAIL_DOMAINempty → fail fastROOM_MEMBERS_LIMIT/ROOM_MEMBERS_CALL_LIMITnon-positive → fail fast
I can create it now and assign it to you if that would be helpful.
🧠 Learnings used
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/roomkeysender/roomkeysender.go:28-30
Timestamp: 2026-04-08T08:25:32.108Z
Learning: In this repository’s Go code, follow the CLAUDE.md rule: don’t add defensive error handling, fallbacks, or nil/validation guards for conditions that are guaranteed not to occur. For internal functions/methods (e.g., when all callers are known to pass non-nil pointer parameters, such as pkg/roomkeysender/roomkeysender.go’s Send), the absence of nil-pointer guards is acceptable—do not flag it as a review issue. Reserve pointer validation and other safety checks for system boundaries (e.g., external inputs, API/HTTP requests, deserialization, or other untrusted entry points).
Learnt from: Joey0538
Repo: hmchangw/chat PR: 59
File: pkg/model/model_test.go:306-317
Timestamp: 2026-04-08T08:25:48.132Z
Learning: In Go, `time.Time` is `comparable` and therefore may be used as a type argument to generic constraints requiring `comparable` (e.g., `func roundTrip[T comparable](...)`). This is because `time.Time`’s underlying fields are all comparable (`uint64`, `int64`, and a pointer `*Location`, where pointers are comparable by address). As a result, any struct that contains or embeds a `time.Time` field (e.g., `model.Message`, or structs embedding it like `model.NotificationEvent`) should also satisfy `comparable` and should not be flagged as a compilation-error when used with `T comparable`.
Learnt from: general-lex
Repo: hmchangw/chat PR: 141
File: message-worker/main.go:95-99
Timestamp: 2026-05-04T03:30:38.678Z
Learning: In hmchangw/chat, `pkg/natsutil/connect.go`’s `natsutil.Connect` returns an OpenTelemetry-wrapped NATS connection (`*otelnats.Conn` from `github.com/Marz32onE/instrumentation-go/otel-nats/otelnats`), not a plain `*nats.Conn`. Because `otelnats.Conn.Publish` is defined as `Publish(ctx context.Context, subj string, data []byte) error` (it uses `ctx` to propagate traces via NATS headers), code review should NOT flag `nc.Publish(ctx, subj, data)` as an incorrect method call shape. This `Publish(ctx, ...)` pattern is the standard/correct usage across the codebase (e.g., message-worker, broadcast-worker, notification-worker, history-service).
Learnt from: general-lex
Repo: hmchangw/chat PR: 205
File: history-service/internal/service/pin.go:66-83
Timestamp: 2026-05-21T05:30:30.938Z
Learning: When reviewing cap enforcement in hmchangw/chat within history-service and room-service (e.g., functions like enforcePinLimit in history-service/internal/service/pin.go using MAX_* limits), the code may intentionally use a non-atomic read-then-write “soft cap” (allowing a small TOCTOU overage). Do not flag it as a TOCTOU/race-condition bug if the cap is explicitly designed to be operationally bounded rather than a hard invariant—i.e., it self-corrects because subsequent requests observe the inflated count and are rejected, and the codebase has not chosen atomic enforcement (e.g., LWT/counter table or findAndModify) as a requirement. Only flag as a TOCTOU issue when the surrounding codebase/design explicitly adopts atomic cap enforcement for these limits.
Learnt from: general-lex
Repo: hmchangw/chat PR: 247
File: room-service/handler.go:1506-1559
Timestamp: 2026-06-01T09:01:36.432Z
Learning: In hmchangw/chat’s room-service cross-site handlers, it’s intentional to (1) perform the local MongoDB write first (DB is the source of truth), then (2) publish the cross-site OUTBOX event, and (3) return an error to the client if the publish step fails—because clients are expected to debounce/retry. During review, do not flag individual handlers that follow this uniform pattern as incorrect or missing a “durable outbox” implementation; the durable-outbox work (separate Mongo outbox collection, new store method, idempotency keys, background drainer, DLQ/dead-letter handling) is tracked as a separate service-wide initiative. Only raise issues if a handler deviates from this established sequence/behavior (e.g., writes after publishing, or hides publish failures inconsistent with this design).
Learnt from: Allan-Code-hub
Repo: hmchangw/chat PR: 263
File: upload-service/main.go:111-115
Timestamp: 2026-06-03T03:26:01.642Z
Learning: In this repo’s Go code, `mongoutil.Disconnect(ctx context.Context, client *mongo.Client)` returns no value and internally logs disconnect errors (e.g., via `slog.Error("mongo disconnect failed", ...)`). When reviewing, never suggest `return mongoutil.Disconnect(...)`—it will not compile. Instead, call it as a statement (`mongoutil.Disconnect(ctx, client)`) and then return `nil` (or the appropriate value for the surrounding function) if needed. This call pattern should be consistent across services’ shutdown paths (e.g., `*/main.go`).
Learnt from: hmchangw
Repo: hmchangw/chat PR: 256
File: room-service/handler.go:709-713
Timestamp: 2026-06-12T00:02:37.654Z
Learning: In hmchangw/chat, NATS events and subscription guard timestamps use the shared `Timestamp int64` convention: set from `time.Now().UTC().UnixMilli()` (millisecond resolution). High-water-mark guards (e.g., `lastSeenAt` read-receipts, `rolesUpdatedAt`, `muteUpdatedAt`) intentionally use strict `$lt`/“only advance when newer” semantics; an equal-millisecond timestamp resulting in a guard no-op (second mutation in the same millisecond) is intentional, tested behavior that should self-heal on the next mutation. During review, do NOT flag millisecond-precision timestamp collisions/equal-millisecond no-ops as bugs in guard logic. If the code changes the shared `Timestamp` ordering strategy (e.g., nanosecond key or per-subscription atomic sequence), that is a coordinated future initiative requiring updates to the shared convention in `pkg/model` and every guard site (inbox-worker and room-service).
Collaborator
Author
|
Also hardened the new Teams subject builders in 57a2b50: the concrete |
Collaborator
Author
|
Per the maintainer decision: converted the 3 Teams subject builders ( |
chenjr0719
force-pushed
the
feat/teams-integration
branch
from
a29143b to
7479d57
Compare
chenjr0719
force-pushed
the
feat/teams-integration
branch
from
b948d86 to
f19a7e9
Compare
mliu33
approved these changes
Jun 17, 2026
mliu33
left a comment
mliu33
left a comment
Collaborator
There was a problem hiding this comment.
Outstanding work! Just some minor comments, thanks!
| ##### Triggered events — success path | ||
|
|
||
| On first creation, a `teams_meet_started` system message is published on the canonical message path (`chat.msg.canonical.{siteID}.created`), persisted by `message-worker`, and fanned out to room members like other system messages. Its `sysMsgData` carries `{ "meetingId": "...", "joinUrl": "..." }`. | ||
| On idempotent repeat calls that return cached meeting details, no additional system message is published. |
| RoomID: roomID, | ||
| UserAccount: byAccount, | ||
| Type: model.MessageTypeTeamsMeetStarted, | ||
| Content: "started a Teams meeting", |
Collaborator
There was a problem hiding this comment.
Shouldn't this be "xxx started a Teams meeting" where xxx is user display name (check add member system message for example)
| // the same meeting for repeated/concurrent calls with the same key, so even | ||
| // a true race never produces two distinct Graph meetings (the correctness | ||
| // guarantee across restarts and multiple service instances). | ||
| // 2. A teams_meetings Mongo record with a UNIQUE index on (roomId, siteId) — |
Collaborator
There was a problem hiding this comment.
Can we simplify the comments here? It's too long
Add three NATS RPC handlers in room-service: - teams.call (calls/room): Teams deep link for a room call, self excluded. - teams.call.user (calls/user): Teams deep link for a 1:1 call. - teams.meeting (meetings): create a Graph onlineMeeting, idempotent per room. Email is derived as account@TEAMS_EMAIL_DOMAIN (no user-service lookup); the requester comes from the NATS subject. The meetings RPC reads back the last teams_meet_started system message (keyspace-aware) for idempotency and writes the marker via the canonical message path. Adds pkg/msgraph (client-credentials token + CreateOnlineMeeting behind a mockable interface) and pkg/model contracts. Closes hmchangw#314 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… doc race - pkg/msgraph: parse the Graph error envelope and surface only status + error.code; never wrap the raw response body into the error/cause. - pkg/subject: validate the account token (reject NATS wildcards) in the Teams concrete subject builders, matching the existing room-scoped builders. - docs/client-api.md: clarify teams_meet_started is emitted only on first creation, not on idempotent cache hits. - room-service: document the known read-then-create idempotency race at the meetings handler (eventually-idempotent via the marker; no lock primitive in this layer — left for maintainer scope decision). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per the shared-pkg no-panic policy (F12 / hmchangw#224 / hmchangw#215): TeamsRoomCall, TeamsMeeting, and TeamsUserCall now return (string, error) and return an error on an invalid (NATS-wildcard) account token instead of panicking. The wildcard check itself is unchanged. These concrete builders have no callers yet (only the *Pattern variants are wired), so there is no cascade. Sibling builders (RoomAppTabs, etc.) are left as-is — new code adopts the preferred form. Adds subject tests for the happy path and the error path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two concurrent teams.meeting calls for the same room could each create a Graph onlineMeeting and each publish a teams_meet_started system message: the prior idempotency read scanned message buckets for the last marker, which is written asynchronously, so racing callers saw no marker yet. Push idempotency to two cooperating layers: - pkg/msgraph: switch onlineMeeting creation to Graph's createOrGet endpoint with a stable per-room externalId (siteID:roomID). Graph guarantees one meeting per (organizer, externalId), so concurrent calls return the same meeting — correctness across restarts/instances with no local lock. externalId is now required. - room-service: replace the Cassandra message-bucket marker scan with a first-class teams_meetings Mongo record, UNIQUE on (roomId, siteId). This mirrors the room_members / subscriptions retry-safe-write convention (unique index + IsDuplicateKeyError == success). The meetings handler now: fast-path reads the record, else createOrGet, then inserts the record; on a duplicate-key race the loser reads back the winner's record and returns it WITHOUT publishing a second system message. The teams_meet_started message is published only by the insert winner — exactly once per room. Removes the now-unused cassMeetMarkerReader and its MESSAGE_BUCKET_HOURS / MESSAGE_READ_MAX_BUCKETS config (room-service no longer scans buckets for meetings). Tests: msgraph httptest asserts the createOrGet path + externalId in the body and that a repeat returns the same meeting; room-service covers fresh create, fast-path read-hit, duplicate-key read-back, and a race-detector concurrent test asserting one Graph meeting + one system message across 8 goroutines. Co-Authored-By: Claude <noreply@anthropic.com>
…esting) Documents pkg/msgraph: env config (TEAMS_*), the client-credentials token flow, the createOrGet+externalId idempotency, the production application-access-policy gate, and the credential-free httptest pattern. Linked from docs/client-api.md (doc-ratchet).
…eway HTTP path, not a route served here
The httptest token-server mocks encode tokenResponse{AccessToken:"tok"...};
gosec G117 flags the marshaled access_token field as a secret pattern. These are
dummy values in test doubles of Microsoft's OAuth endpoint, not real secrets —
annotated with // #nosec G117 + justification per repo SAST policy.
…me + trim comments Address review feedback on the Teams meeting handler: - sys message Content now formats `"<display>" started a Teams meeting` (matches the room_restricted / members_added sys-message convention), falling back to the account when the user lookup fails. - condense the long idempotency doc-block and inline step comments.
chenjr0719
force-pushed
the
feat/teams-integration
branch
from
f19a7e9 to
0558e61
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@room-service/store_mongo.go`:
- Around line 205-209: The error returned from s.teamsMeetings.InsertOne at line
209 is returned bare instead of being wrapped with context, which violates the
project's error-wrapping guidelines. Modify the return statement to wrap the
error using fmt.Errorf with a descriptive message and the error value using the
%w verb (e.g., fmt.Errorf("insert teams meeting record: %w", err)). This
wrapping maintains compatibility with the upstream mongo.IsDuplicateKeyError
check since errors.As can unwrap fmt.Errorf-wrapped errors while providing
necessary operation context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed0c4b8e-d4c9-4c31-b53d-88455ff02e2b
📒 Files selected for processing (18)
docs/client-api.mddocs/msgraph-client.mdpkg/model/event.gopkg/model/message.gopkg/model/teams.gopkg/msgraph/msgraph.gopkg/msgraph/msgraph_test.gopkg/subject/subject.gopkg/subject/subject_test.goroom-service/deploy/docker-compose.ymlroom-service/handler.goroom-service/handler_teams.goroom-service/handler_teams_test.goroom-service/helper.goroom-service/main.goroom-service/mock_store_test.goroom-service/store.goroom-service/store_mongo.go
✅ Files skipped from review due to trivial changes (2)
- room-service/deploy/docker-compose.yml
- docs/msgraph-client.md
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/model/message.go
- pkg/subject/subject_test.go
- room-service/mock_store_test.go
- room-service/store.go
- room-service/main.go
- room-service/helper.go
- pkg/model/teams.go
- pkg/msgraph/msgraph_test.go
- pkg/msgraph/msgraph.go
- docs/client-api.md
- room-service/handler.go
- room-service/handler_teams_test.go
- room-service/handler_teams.go
Per the repo error-wrapping rule. mongo.IsDuplicateKeyError unwraps via errors.As, so the handler's idempotency-race dup-key check still works.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the Microsoft Teams integration from #314 as three NATS RPC handlers in
room-service. Two are pure Teams deep-link builders (no Graph); one creates a GraphonlineMeetingand is idempotent per room. Built against mocks — the maintainer runs the live Azure test (no creds held here).Closes #314
What's included
pkg/model— request/reply contracts (TeamsRoomCallRequest,TeamsUserCallRequest,TeamsCallReply,TeamsMeetingRequest,TeamsMeetingReply),MessageTypeTeamsMeetStarted = "teams_meet_started", andTeamsMeetStartedSysData{meetingId, joinUrl}.pkg/msgraph— minimal Graph client: client-credentials (app-only) token with caching +CreateOnlineMeeting, behind aClientinterface so the meetings RPC is unit-testable without Azure.room-service— three handlers registered inHandler.Register:teams.call(calls/room):ListRoomMembers→account@TEAMS_EMAIL_DOMAINexcluding self →https://teams.microsoft.com/l/call/0/0?users=...→{joinUrl}. EnforcesROOM_MEMBERS_CALL_LIMIT(20).teams.call.user(calls/user):accountName@domain→ same deep-link shape →{joinUrl}.teams.meeting(meetings): validate membership/room → fast-path read theteams_meetingsrecord (idempotent return if present) → elseListRoomMembers(enforceROOM_MEMBERS_LIMIT500) → GraphcreateOrGet(externalId =siteID:roomID) → insert theteams_meetingsrecord (unique onroomId+siteId; on duplicate-key, read back the concurrent winner and return without publishing) → on first creation, publishteams_meet_startedvia the canonical message path →{id, joinUrl}.createOrGetkeyed on a stable per-roomexternalId(siteID:roomID) guarantees one meeting per room from the source of truth; a first-classteams_meetingsMongo record with a UNIQUE index on(roomId, siteId)guards local state so exactly oneteams_meet_startedsystem message is published per room (TeamsMeetingStoreinroom-service/store.go,MongoStore.GetTeamsMeeting/InsertTeamsMeeting+ the unique index inroom-service/store_mongo.go). Any remaining Cassandra reads stay keyspace-bound (the gocql session is bound toCASSANDRA_KEYSPACE; never hardcodeschat/tchat).TEAMS_TENANT_ID/CLIENT_ID/CLIENT_SECRET,TEAMS_EMAIL_DOMAIN,ROOM_MEMBERS_LIMIT(500),ROOM_MEMBERS_CALL_LIMIT(20). (The previousMESSAGE_BUCKET_HOURS/MESSAGE_READ_MAX_BUCKETSknobs are removed — room-service no longer scans message buckets for meetings.)docs/client-api.mdgains the 3 calls (subjects, bodies, responses, error tables) + the RPC index rows.NATS subjects (chosen)
chat.user.{account}.request.room.{roomID}.{siteID}.teams.callchat.user.{account}.request.teams.{siteID}.call.userchat.user.{account}.request.room.{roomID}.{siteID}.teams.meetingRejected alternative: the issue sketched
chat.user.{account}.request.teams.{siteID}.{room|user|meeting}. The room-scoped endpoints instead carryroom.{roomID}.{siteID}sonatsrouterextracts{account}+{roomID}exactly like every other room RPC (member.list,mute.toggle, …) andrequireMembershipAndGetRoomworks unchanged.calls/userhas no room, so it stays user-scoped with the target in the body.Design rationale (idempotency)
Two concurrent
teams.meetingcalls for the same room previously could each create a Graph meeting and each publish ateams_meet_startedsystem message: the old idempotency read scanned message buckets for the last marker, and the marker is written asynchronously via the canonical message path, so racing callers saw no marker yet (documented as a known race in the prior code). The fix pushes idempotency to two cooperating layers:Graph
createOrGet= idempotency at the source of truth.POST /users/{organizerId}/onlineMeetings/createOrGetwith anexternalId(pkg/msgraph/msgraph.goCreateOnlineMeeting) returns one meeting per(organizer, externalId). TheexternalIdis the stable per-room keysiteID:roomID(room-service/handler_teams.goteamsMeetingExternalID). This holds across process restarts and multiple service instances with no local lock — even a true race returns the same Graph meeting to both callers.Mongo unique-key record (NOT Cassandra) = local single-publish guard. room-service's primary store is Mongo, and it already uses unique-index +
mongo.IsDuplicateKeyError-as-success as its retry-safe-write idempotency convention — see theroom_members(rid, member.type, member.id)andsubscriptions(roomId, u.account)unique indexes inroom-service/store_mongo.goEnsureIndexes(the room-worker bulk-insert path treats the dup-key error as success for the same reason). This change adds ateams_meetingscollection with a UNIQUE index on(roomId, siteId)in the same style. The handler inserts the record aftercreateOrGet; on a concurrent insert the loser hits the unique constraint, reads back the winner's record, and returns without publishing — so exactly oneteams_meet_startedis published per room. The record also makes the meeting a queryable first-class document instead of scanning message buckets.Cassandra in room-service is a read-only message-history reader (
room-service/store_cassandra.goonlySELECTs); a Cassandra LWT/PaxosIF NOT EXISTSwould introduce a new, heavier consensus primitive the service doesn't otherwise use — the wrong tool when the store the service owns already has the exact idempotency convention needed.Self-audit (pre-review)
Message-path trace: 1 client-req ✅ — Teams RPC structs (
pkg/model/teams.go) +docs/client-api.md· 2 gatekeeper N/A — room-service NATS RPC, not the message-gatekeeper pipeline · 3 model ✅ —pkg/model/teams.go+MessageTypeTeamsMeetStarted(pkg/model/event.go) +TeamsMeetStartedSysData(pkg/model/message.go) · 4 workers ✅ no-change —message-workerpersists any non-emptyType· 5 storage ✅ — first-classteams_meetingscollection, UNIQUE index(roomId, siteId)(room-service/store_mongo.goEnsureIndexes) · 6 read ✅ —MongoStore.GetTeamsMeetingfast-path read (room-service/store_mongo.go) · 7 client-events ✅ —teams_meet_startedvia existing system-message fan-outChecklist — item · evidence (file) · source-of-truth (ref / prior review)
pkg/model/teams.go(5 structs) · ref F7 (feat: channel rename and visibility RPCs #224 / docs(superpowers): add client-api doc review design spec #216 / refactor(cassandra): v3 reactions — embedded MAP<reaction_key, reactor_info> (read+storage) #221)omitempty, required fields not ·pkg/model/teams.go(roomId,omitemptyvsaccountName/joinUrl/id) · ref A1 (feat(model): unify pin/unpin room events into single struct with pinned field #300)room-service/store_cassandra.go(FROM messages_by_room, no keyspace prefix; session bound toCASSANDRA_KEYSPACE) · ref §C (thechat/tchatkeyspace-mismatch incident)docs/client-api.mdsame PR · ref §Droom-service/handler_teams.go· ref F9 (feat: channel rename and visibility RPCs #224 / feat: implement user-presence-service with cross-site NATS pub/sub #261)createOrGet(externalIdsiteID:roomID,pkg/msgraph/msgraph.goCreateOnlineMeeting) +teams_meetingsUNIQUE(roomId, siteId)withmongo.IsDuplicateKeyErrorread-back, system message published only by the insert winner ·room-service/handler_teams.goteamsMeeting,room-service/store_mongo.go· ref F2 (feat: add BotDM re-subscribe upsert with DisableNotification field #202 / feat: real-time thread reply fan-out (broadcast-worker) + reply-count badge pipeline #245)pkg/— Teams subject builders return(string, error)on invalid token (converted from panic) ·pkg/subject/subject.go· ref F12 (feat: channel rename and visibility RPCs #224 / feat: history-service JetStream edit/delete + unify canonical Nats-Msg-Id across publishers #215)siteIDin request-routed subjects only, no broadcast subject) · F4 / F6 / F13 (no Mongo projection / singleflight / push-worker touched)Decisions & rationale (with source)
account@TEAMS_EMAIL_DOMAIN, no user-service lookup — at the NATS layer onlyaccountis available; the OIDC email claim exists only at auth-service token-exchange (auth-service/handler.go:178), and dev already derivesaccount + "@dev.local"(auth-service/handler.go:220). Open gate: prodaccount(=claims.PreferredUsername,auth-service/handler.go:152) must equal the email local-part under one domain.## Design rationale (idempotency)section below. GraphcreateOrGetis the cross-instance correctness guarantee; theteams_meetingsunique index is the local single-publish guard, reusing room-service's existing unique-index +IsDuplicateKeyErrorconvention. Source: ref F2.chat.user.{account}.request.room.{roomID}.{siteID}.teams.{call|meeting}→requireMembershipAndGetRoom), flat for the 1:1 user call — matches every existing room RPC. Source: ref F1 / F11.Source-of-truth discipline: every ✅/citation above verified against the changed files in this branch; ref ids (A/C/D/F-series) map to the project's mined review history.
Verification
go build ./...,go vet ./..., andgo test ./room-service/... ./pkg/msgraph/... ./pkg/model/...all pass locally, including the new idempotency/concurrency tests (a-racerun of the concurrent meetings test is clean). Live Azure/Graph test is for the maintainer.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
🤖 Generated with Claude Code