Conversation
… viewer relations Move all artifact FGA update_access logic into PublishPastMeetingEvent so that recording_access, transcript_access, and ai_summary_access (which live on the past meeting record) drive the FGA tuples rather than being scattered across per-artifact publishers. New named relations on v1_past_meeting — recording_viewer, transcript_viewer, ai_summary_viewer — support both direct user:* assignment (public) and role-based access via self-referential past_meeting_for_*_view tuples (host/attendee/participant). The indexer's access_check_relation is updated to match for each artifact type. FGA blocks removed from PublishPastMeetingRecordingEvent, PublishPastMeetingTranscriptEvent, and PublishPastMeetingSummaryEvent; they now only publish the indexer message. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConsolidates FGA access publishing for past-meeting artifacts: a single publisher now emits per-artifact relations ( Changes
Sequence Diagram(s)sequenceDiagram
participant Publisher as Publisher\n(PublishPastMeetingEvent)
participant FGA as FGA Service
participant Indexer as Indexer Queue
participant ArtifactPub as Artifact Publishers\n(Recording/Transcript/Summary)
Publisher->>FGA: Publish FGA update_access\nUID = MeetingAndOccurrenceID\nRelations: recording_viewer, transcript_viewer, ai_summary_viewer\nReference tuples: v1_past_meeting:<MeetingAndOccurrenceID>\nExcludeRelations: host,invitee,attendee
FGA-->>Publisher: Ack
Publisher->>Indexer: Publish indexer messages for artifacts\n(updated AccessCheckRelation per artifact)
ArtifactPub->>Indexer: Publish indexer-only messages\n(no FGA call)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/infrastructure/eventing/nats_publisher.go (1)
436-454: 🛠️ Refactor suggestion | 🟠 MajorAccess-only updates still leave the artifact
Publicbit stale in the indexer.The FGA tuples now move with
PublishPastMeetingEvent, butIndexingConfig.Publicfor recordings/transcripts/summaries is still only recomputed when each artifact is republished. After a parent past-meeting access change, authorization and search visibility can diverge until every artifact document is reindexed.Also applies to: 472-489, 508-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/eventing/nats_publisher.go` around lines 436 - 454, The indexer "Public" bit for child artifacts (recordings/transcripts/summaries) isn't updated when a parent past-meeting access change is handled by PublishPastMeetingEvent, so search/authorization can diverge; update PublishPastMeetingEvent to recompute IndexingConfig.Public for all child artifacts and emit indexer updates (use p.publish) for IndexV1PastMeetingRecordingSubject, IndexV1PastMeetingTranscriptSubject and IndexV1PastMeetingSummarySubject (or the same indexer message path used elsewhere) with an indexerMsg that sets the corrected IndexingConfig.Public value (keeping existing identifiers like recording.MeetingAndOccurrenceID, ParentRefs(), SortName(), NameAndAliases(), Fulltext() as needed) so the artifact documents’ Public flag is kept in sync without requiring full republish.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/infrastructure/eventing/nats_publisher.go`:
- Around line 330-338: The past-meeting update_access message in
nats_publisher.go (constructed as pastMeetingAccessMsg with GenericAccessData
and pastMeetingRelations) currently includes host/invitee/attendee tuples and
can overwrite memberships managed separately by
PublishPastMeetingParticipantEvent/member_put; modify the code that builds or
assigns pastMeetingRelations (used in pastMeetingAccessMsg) to filter out
relation types "host", "invitee", and "attendee" (i.e., remove any relation
entries where Relation == "host" || "invitee" || "attendee") so the
update_access publish does not wipe those membership tuples while leaving
pastMeetingRefs unchanged.
- Around line 293-337: The code uses meeting.ID for the FGA UID and selfRef but
the rest of the system expects the artifact tuples on the
meeting_and_occurrence_id; update all usages of meeting.ID in this block to the
meeting.MeetingAndOccurrenceID field (use the exact struct field name
MeetingAndOccurrenceID) so UID in GenericAccessData and selfRef
("v1_past_meeting:"+meeting.MeetingAndOccurrenceID) point to the same
v1_past_meeting object where PublishPastMeetingParticipantEvent stores
host/attendee/invitee tuples; ensure you update pastMeetingAccessMsg.Data.UID
and every pastMeetingRefs assignment to reference the new selfRef value.
---
Outside diff comments:
In `@internal/infrastructure/eventing/nats_publisher.go`:
- Around line 436-454: The indexer "Public" bit for child artifacts
(recordings/transcripts/summaries) isn't updated when a parent past-meeting
access change is handled by PublishPastMeetingEvent, so search/authorization can
diverge; update PublishPastMeetingEvent to recompute IndexingConfig.Public for
all child artifacts and emit indexer updates (use p.publish) for
IndexV1PastMeetingRecordingSubject, IndexV1PastMeetingTranscriptSubject and
IndexV1PastMeetingSummarySubject (or the same indexer message path used
elsewhere) with an indexerMsg that sets the corrected IndexingConfig.Public
value (keeping existing identifiers like recording.MeetingAndOccurrenceID,
ParentRefs(), SortName(), NameAndAliases(), Fulltext() as needed) so the
artifact documents’ Public flag is kept in sync without requiring full
republish.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8d1b8b2-5352-4f1a-926a-2fbf5794a91a
📒 Files selected for processing (2)
docs/indexer-contract.mdinternal/infrastructure/eventing/nats_publisher.go
There was a problem hiding this comment.
Pull request overview
This PR updates authorization for past meeting artifacts to use per-artifact viewer relations on the v1_past_meeting object, and centralizes FGA access tuple publishing into the past meeting publisher so access stays in sync when the past meeting record changes.
Changes:
- Add per-artifact viewer tuple publishing (
recording_viewer,transcript_viewer,ai_summary_viewer) toPublishPastMeetingEvent. - Update indexer
access_check_relationfor recording/transcript/summary artifacts to use the new past-meeting relations. - Remove per-artifact FGA
update_accesspublishing from recording/transcript/summary publishers (they now only publish indexer messages).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/infrastructure/eventing/nats_publisher.go | Centralizes past-meeting artifact access tuple publishing into the past meeting publisher and updates artifact indexer access-check relations. |
| docs/indexer-contract.md | Updates the indexer contract to reflect the new access-check relations for past meeting artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…uples - Use MeetingAndOccurrenceID (not ID) as the FGA object UID and selfRef, so artifact-view tuples land on the same v1_past_meeting object where PublishPastMeetingParticipantEvent stores host/attendee/invitee memberships. - Add ExcludeRelations: ["host", "invitee", "attendee"] to the update_access message so past-meeting republish does not wipe participant tuples managed separately by PublishPastMeetingParticipantEvent. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <[email protected]>
Contributor
Author
|
Addressed the two inline comments in the follow-up commit (143f018):
Re: indexer |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/infrastructure/eventing/nats_publisher.go (1)
293-296:⚠️ Potential issue | 🔴 CriticalMissing validation for
MeetingAndOccurrenceIDwill create malformed FGA tuples.
PublishPastMeetingParticipantEventvalidatesMeetingAndOccurrenceID(lines 353-355) before using it, but this function uses it directly at line 295 without validation. If empty,selfRefbecomes"v1_past_meeting:"and the FGA message at line 334 will have an empty UID.🐛 Proposed fix to add validation
// Per-artifact access: self-referential references enable role-based access // via the existing host/attendee/invitee tuples on the same v1_past_meeting object. + if meeting.MeetingAndOccurrenceID == "" { + return domain.NewValidationError("meeting_and_occurrence_id is required for past meeting FGA access update") + } selfRef := "v1_past_meeting:" + meeting.MeetingAndOccurrenceIDAs per coding guidelines, "internal/**/*.go: Use domain-specific error types defined in
internal/domain/errors.gofor error handling throughout the application".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/eventing/nats_publisher.go` around lines 293 - 296, The code constructs selfRef := "v1_past_meeting:"+meeting.MeetingAndOccurrenceID without validating MeetingAndOccurrenceID, which can produce an empty UID; add an explicit non-empty check for meeting.MeetingAndOccurrenceID at the top of the function that builds selfRef (same validation logic as PublishPastMeetingParticipantEvent) and return the appropriate domain-specific validation error from internal/domain/errors.go when it's empty; ensure the check runs before forming selfRef and before any FGA tuple/message construction (references: selfRef and PublishPastMeetingParticipantEvent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/infrastructure/eventing/nats_publisher.go`:
- Around line 293-296: The code constructs selfRef :=
"v1_past_meeting:"+meeting.MeetingAndOccurrenceID without validating
MeetingAndOccurrenceID, which can produce an empty UID; add an explicit
non-empty check for meeting.MeetingAndOccurrenceID at the top of the function
that builds selfRef (same validation logic as
PublishPastMeetingParticipantEvent) and return the appropriate domain-specific
validation error from internal/domain/errors.go when it's empty; ensure the
check runs before forming selfRef and before any FGA tuple/message construction
(references: selfRef and PublishPastMeetingParticipantEvent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 007eb0ad-1e23-4bab-a19c-8fdc62a8d540
📒 Files selected for processing (1)
internal/infrastructure/eventing/nats_publisher.go
…_meeting
The summary GET rule was checking viewer on v1_past_meeting_summary, which no
longer has access tuples written to it since FGA was consolidated onto
v1_past_meeting in this PR.
- GET: relation: ai_summary_viewer, object: v1_past_meeting:{past_meeting_id}
- PUT: relation: organizer, object: v1_past_meeting:{past_meeting_id}
(organizer already delegates correctly from v1_past_meeting)
Generated with [Claude Code](https://claude.ai/code)
Signed-off-by: Andres Tobon <[email protected]>
… relations fga-sync prepends "user:" to every principal in the Relations map, so sending "user:*" produced "user:user:*" which is invalid. Changed all three public-access artifact relations (recording_viewer, transcript_viewer, ai_summary_viewer) to send "*" instead, yielding the correct "user:*" tuple. Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Andres Tobon <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/infrastructure/eventing/nats_publisher.go (1)
293-340:⚠️ Potential issue | 🟠 MajorFail fast when
meeting_and_occurrence_idis missing.Like
PublishPastMeetingParticipantEvent, Line 295 and Line 334 now rely onmeeting.MeetingAndOccurrenceID. If it is empty, this publishes an emptyUIDandv1_past_meeting:self-references after the indexer event may already have been sent.🛡️ Suggested fix
func (p *NATSPublisher) PublishPastMeetingEvent(ctx context.Context, action string, meeting *models.PastMeetingEventData) error { + if meeting.MeetingAndOccurrenceID == "" { + return domain.NewValidationError("meeting_and_occurrence_id is required for past meeting access update") + } + p.logger.InfoContext(ctx, "publishing past meeting event", "action", action, "past_meeting_id", meeting.ID)As per coding guidelines, "internal/**/*.go: Use domain-specific error types defined in
internal/domain/errors.gofor error handling throughout the application"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/eventing/nats_publisher.go` around lines 293 - 340, Check for an empty meeting.MeetingAndOccurrenceID early and fail fast before building fgatypes.GenericFGAMessage or creating selfRef (the same guard used in PublishPastMeetingParticipantEvent); if empty, return a domain-specific error from internal/domain/errors.go (e.g., ErrMissingMeetingAndOccurrenceID) instead of publishing an empty UID/self-reference so the indexer event never gets sent with invalid data. Ensure the check is placed before any use of meeting.MeetingAndOccurrenceID and that the function returns the appropriate domain error type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/infrastructure/eventing/nats_publisher.go`:
- Around line 272-274: The indexer currently leaves artifact documents' Public
boolean stale when parent past-meeting access flags change, which can cause
visibility bugs if search queries trust Public; update the behavior in
PublishPastMeetingEvent (and related nats_publisher logic) to either a) ensure
downstream search does not treat artifact.Public as authoritative by forcing
queries to evaluate AccessCheckObject against FGA (confirm and document this
contract where search code reads artifacts), or b) implement a republish path
inside PublishPastMeetingEvent that enqueues or triggers the existing artifact
publisher(s) to re-index artifacts (recording/transcript/ai_summary) when
recording_access/transcript_access/ai_summary_access change; locate references
to Public on artifact documents, AccessCheckObject checks, and the artifact
publisher functions to wire the chosen fix.
---
Duplicate comments:
In `@internal/infrastructure/eventing/nats_publisher.go`:
- Around line 293-340: Check for an empty meeting.MeetingAndOccurrenceID early
and fail fast before building fgatypes.GenericFGAMessage or creating selfRef
(the same guard used in PublishPastMeetingParticipantEvent); if empty, return a
domain-specific error from internal/domain/errors.go (e.g.,
ErrMissingMeetingAndOccurrenceID) instead of publishing an empty
UID/self-reference so the indexer event never gets sent with invalid data.
Ensure the check is placed before any use of meeting.MeetingAndOccurrenceID and
that the function returns the appropriate domain error type.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d648b0d5-c291-406a-a5b7-b408900b3b9e
📒 Files selected for processing (1)
internal/infrastructure/eventing/nats_publisher.go
prabodhcs
reviewed
Apr 15, 2026
|
|
||
| // Per-artifact access: self-referential references enable role-based access | ||
| // via the existing host/attendee/invitee tuples on the same v1_past_meeting object. | ||
| selfRef := "v1_past_meeting:" + meeting.MeetingAndOccurrenceID |
There was a problem hiding this comment.
I see meeting.MeetingAndOccurrenceID is used multiple times, also for the access. Can we add a validation
if meeting.MeetingAndOccurrenceID == "" {
return domain.NewValidationError("meeting_and_occurrence_id is required for past meeting FGA access update")
}
Contributor
Author
There was a problem hiding this comment.
That's a good point. And actually when we send the indexer message using meeting.ID, that is the same value as the meeting.MeetingAndOccurrenceID, so I switched to using meeting.MeetingAndOccurrenceID in all places in the code and moved the validation to the top of the function. There aren't any cases where this validation has failed currently since all meetings have an ID, but it is still a good safety check.
prabodhcs
previously approved these changes
Apr 15, 2026
…D explicitly Use meeting.MeetingAndOccurrenceID instead of meeting.ID in log and IndexingConfig fields. Also add a validation guard so publishing fails early with a clear error when MeetingAndOccurrenceID is empty. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Andres Tobon <[email protected]>
andrest50
force-pushed
the
fix/LFXV2-1505-artifact-access-check-object-v2
branch
from
8aa79f8 to
8f12040
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/infrastructure/eventing/nats_publisher.go (2)
243-247: Move the info log below the validation guard.If
MeetingAndOccurrenceIDis empty, this still emits a"publishing past meeting event"log even though the function returns immediately. Logging after the guard keeps the audit trail aligned with actual publish attempts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/eventing/nats_publisher.go` around lines 243 - 247, The info log is emitted before the validation guard, causing "publishing past meeting event" to be logged even when MeetingAndOccurrenceID is empty; move the p.logger.InfoContext(ctx, "publishing past meeting event", "action", action, "past_meeting_id", meeting.MeetingAndOccurrenceID) call so it occurs after the if check that returns domain.NewValidationError("meeting_and_occurrence_id is required for publishing messages about the past meeting"), ensuring the log only runs when meeting.MeetingAndOccurrenceID is present and a publish is actually attempted.
301-332: Consider extracting the per-artifact access mapping into a helper.These three
switchblocks are structurally identical, so the next relation rename or access-policy tweak will have to stay in sync in three places. A small helper here would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/infrastructure/eventing/nats_publisher.go` around lines 301 - 332, The three identical switch blocks handling meeting.RecordingAccess, meeting.TranscriptAccess, and meeting.AISummaryAccess should be extracted into a helper to avoid duplication and drift; create a function (e.g., applyArtifactAccess(access string, artifactPrefix string, pastMeetingRelations map[string][]string, pastMeetingRefs map[string][]string, selfRef string)) that receives the access value and an artifact identifier ("recording", "transcript", "ai_summary") and encapsulates the switch logic to set either pastMeetingRelations[artifact+"_viewer"] = {"*"} for "public" or the three/one pastMeetingRefs keys (e.g., "past_meeting_for_host_<artifact>_view", "past_meeting_for_attendee_<artifact>_view", "past_meeting_for_participant_<artifact>_view" or default host-only) using selfRef; replace the three switch blocks with three calls to applyArtifactAccess(meeting.RecordingAccess, "recording", ...), applyArtifactAccess(meeting.TranscriptAccess, "transcript", ...), and applyArtifactAccess(meeting.AISummaryAccess, "ai_summary", ...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/infrastructure/eventing/nats_publisher.go`:
- Around line 243-247: The info log is emitted before the validation guard,
causing "publishing past meeting event" to be logged even when
MeetingAndOccurrenceID is empty; move the p.logger.InfoContext(ctx, "publishing
past meeting event", "action", action, "past_meeting_id",
meeting.MeetingAndOccurrenceID) call so it occurs after the if check that
returns domain.NewValidationError("meeting_and_occurrence_id is required for
publishing messages about the past meeting"), ensuring the log only runs when
meeting.MeetingAndOccurrenceID is present and a publish is actually attempted.
- Around line 301-332: The three identical switch blocks handling
meeting.RecordingAccess, meeting.TranscriptAccess, and meeting.AISummaryAccess
should be extracted into a helper to avoid duplication and drift; create a
function (e.g., applyArtifactAccess(access string, artifactPrefix string,
pastMeetingRelations map[string][]string, pastMeetingRefs map[string][]string,
selfRef string)) that receives the access value and an artifact identifier
("recording", "transcript", "ai_summary") and encapsulates the switch logic to
set either pastMeetingRelations[artifact+"_viewer"] = {"*"} for "public" or the
three/one pastMeetingRefs keys (e.g., "past_meeting_for_host_<artifact>_view",
"past_meeting_for_attendee_<artifact>_view",
"past_meeting_for_participant_<artifact>_view" or default host-only) using
selfRef; replace the three switch blocks with three calls to
applyArtifactAccess(meeting.RecordingAccess, "recording", ...),
applyArtifactAccess(meeting.TranscriptAccess, "transcript", ...), and
applyArtifactAccess(meeting.AISummaryAccess, "ai_summary", ...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0351ada9-2205-48d0-b457-ba15bfa3f288
📒 Files selected for processing (1)
internal/infrastructure/eventing/nats_publisher.go
prabodhcs
approved these changes
Apr 15, 2026
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jordane
approved these changes
Apr 23, 2026
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
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
update_accesslogic from per-artifact publishers intoPublishPastMeetingEvent, sincerecording_access,transcript_access, andai_summary_accesslive on the past meeting record — not the artifacts.v1_past_meeting:recording_viewer,transcript_viewer,ai_summary_viewer. Each supports directuser:*assignment for public access and role-based access via self-referentialpast_meeting_for_*_viewtuples for host/attendee/participant access.docs/indexer-contract.mdaccess_check_relationfor Recording, Transcript, and Summary to use the new relation names.PublishPastMeetingRecordingEvent,PublishPastMeetingTranscriptEvent, andPublishPastMeetingSummaryEvent— these now only publish the indexer message.Ticket
LFXV2-1505
🤖 Generated with Claude Code