Store visibility on each Conversation

Aperta
#2,380 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
5/5
Tempo stimato
Più di una settimana
Idoneità per principianti
22/100
Tipo di issue
Funzionalità
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
sql, typescript

Direzione di ricerca

Start with chat/conversations/sql/store.ts and the existing tests/component/conversation-sql-store.test.ts scenario named “persists visibility from source signals...”; the issue describes the proposed storage rule and the reader cutover across many files. Follow the listed slices and migrations, then run the component and integration scenarios named in the test plan. Done means root Conversations retain their own visibility, readers use it, and channel privacy changes cascade as specified.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

Step 4 of #2207. It must land before step 5 (GitHub webhook). Step 2 is #2217.

Problem

Conversation visibility is not stored on the Conversation. Readers get it from the shared Destination row of the Slack channel (junior_destinations.visibility). All Conversations in one channel or DM share that one value.

Each write with a visibility signal replaces that value. recordActivity and recordExecution call upsertDestination with refreshVisibility. Dispatches pass destinationVisibility. Slack messages pass the channel type.

This blocks the #2207 use case: work that starts from a public source, such as a comment on a public repository, and runs in the person's private DM.

  • If a dispatch marks the DM public, the DM Destination row becomes public.
  • Then every other Conversation in that DM shows as public. The dashboard shows their transcript and tool data (canExposeConversationPayload). Search, Briefs, and retention also use the public rules.
  • The next normal DM message sets the row back to private. The value keeps changing between the two.

Proposal

Store visibility on each root Conversation. The Destination row keeps the visibility of the Slack channel only.

Storage
  • Add a nullable visibility column to junior_conversations. Values: public, private. NULL means not known yet.
  • Root Conversations own the value. A child Conversation reads the value of its root, as today (root_conversation_id).
  • junior_destinations.visibility stays. It is a fact about the Slack channel. Channel tools (channel-access.ts), Automations, and the Location pages that decide if a Location is named use it. It is no longer the visibility of a Conversation.
Write rule

A Conversation may become more private. It never becomes more public by itself.

Stored Signal Result
NULL public or private the signal
NULL none the current Destination value if it is public or private, else NULL
public private private
public public or none public
private any private
  • One SQL upsert in SqlStore.upsertConversation owns this rule. Other code does not decide it.
  • Why: when the person replies in a public-origin DM Conversation, the reply can contain private data. The Conversation becomes private. A later public event in the same thread must not make it public again.
  • The only path from private to public is an explicit action by a participant, such as the "make public" action in #1545. That action writes the Conversation column, not the Destination row.
  • When a Destination changes from public to private (a Slack channel is converted to private), the same write sets all public Conversations at that Destination to private. This keeps the current protection for converted channels.
  • A Destination that changes from private to public does not change its old Conversations. They stay private. New Conversations there start public. Today they all become public. This is a deliberate fail-closed change.
Dispatch input

Today one dispatch field, destinationVisibility, sets three things: the Destination row, the Conversation, and the Slack thread channelVisibility in slack/dispatch-turn.ts. For a DM these must differ.

  • Keep destinationVisibility as the visibility of the Slack channel. Core sets it to private for a DM.
  • Add an optional conversationVisibility to the dispatch record. It defaults to destinationVisibility. It writes only the Conversation column.
  • dispatchForIdentity (step 2, #2217) stays private-only now. In step 5, it accepts the origin visibility from the plugin, for example the visibility of the GitHub repository. It passes that as conversationVisibility. destinationVisibility stays private.
  • recordActivity and recordExecution take the two values as separate inputs. A dispatch must not write a public value to a DM Destination row.
Fail closed
  • NULL, unknown, or invalid values read as private. resolveConversationPrivacy already does this and logs conversation.visibility.defaulted.
  • Missing or invalid root metadata stays private, as today in privacy.ts and access.ts.
  • Readers do not fall back to the Destination value after the reader cutover. A fallback would bring back the leak.

Migration and backfill

  1. Add the column. In the same migration, copy public or private from the Destination row to each root Conversation that has a destination_id. Leave other values NULL. This is correct now, because no deployed writer can make a DM public yet.
  2. Old workers that run during a deploy do not write the column. Their new rows stay NULL. They read as private, and the next write from a new worker fills them by the rule above.
  3. Before the reader cutover, run the same copy again for rows that are still NULL.

Readers that must change

Each reader below gets visibility from the Destination row of the root Conversation. Each must read the column of the root Conversation instead:

  • chat/conversations/sql/store.ts: conversationFromRow (Conversation.visibility). The agent Turn privacy (chat/agent/index.ts), Briefs input, forks, plugin tasks, resume, and searchConversationEvents use this value.
  • chat/conversations/sql/privacy.ts: resolveRootVisibility (Brief storage and tasks, retention).
  • api/conversations/access.ts: readConversationAccessFromSql (dashboard payload access).
  • api/conversations/list.ts: feed filters and Brief search.
  • api/conversations/stats.query.ts and api/people/profile.query.ts: private titles and counts.
  • api/locations/query.ts: Conversation rows and counts at a Location. The Location itself can still use the Destination value to decide if it is named.
  • chat/conversations/sql/message-search.ts and chat/briefs/search.ts: public cross-Conversation search.
  • chat/conversations/sql/purge.ts: public and private retention windows.
  • policies/data-redaction.md and chat/conversations/README.md: they say that Destination visibility is the privacy authority. Update them.
  • Rename ConversationPrivacy to ConversationVisibility after the cutover, per the TODO in conversation-privacy.ts.

Readers that stay on the Destination value: slack/tool-support/channel-access.ts, automations/read.ts, automations/collection.ts, and resolveDestinationVisibility (it supplies the channel signal for a Slack message).

Slices

  1. Column, migration with backfill, and the write rule in upsertConversation. Conversation.visibility reads the column. Dashboard SQL readers do not change yet. Visible change: an old Conversation stays private in the runtime after its channel becomes public. PR: #2382.
  2. Second backfill for NULL rows. The cascade for channels that become private. Hard cutover of all SQL readers above. Docs and policy update. The cascade writes rows of other Conversations, so it must keep one lock order (Destination row, then Conversation rows).
  3. Split conversationVisibility from destinationVisibility on dispatches and store writes.
  4. Then step 5 of #2207 can pass origin visibility.

Out of scope

  • The reverse case: a private DM run writes to a public issue or PR. Step 3 (#773) and the "no private data in public output" requirement on #2207 track it.
  • Which data a public-origin Conversation may use. In a public-origin Conversation, the person's own credentials can read private data. Tool results then appear in a public transcript. Step 5 must decide if a public-origin Conversation starts private until this is addressed. See open questions.
  • A UI to change visibility (#1545, #1200).

Test plan

  • Extend the primary owning scenario in tests/component/conversation-sql-store.test.ts ("persists visibility from source signals..."). Cover: a signal fills NULL; public → private; no change from private → public; a channel converted to private makes its Conversations private; two Conversations in one DM keep different values.
  • Slice 2: extend the existing dashboard access and feed scenarios (tests/integration/api/conversations/*, tests/integration/dashboard-reporting.test.ts) with one Conversation that differs from its Destination.
  • Slice 3 and step 5: extend tests/integration/identity-dispatch.test.ts (#2217). A public-origin dispatch in a DM shows its own transcript as public. Another Conversation in the same DM stays private.
  • No tests for migration SQL. Use pnpm --filter @sentry/junior db:generate and the migration metadata checks.

Open questions

  1. Is the rule "more private only, except explicit publish" correct? The visible change is that old Conversations stay private after a channel becomes public.
  2. Should a public-origin DM Conversation become private at the first reply by the person (current proposal), or only when private data enters by another path?
  3. Should step 5 start public-origin Conversations as private until the credential question above has an answer?

via @BYK.

--

View Junior Session [Sentry]

Lingua principale
TypeScript
Stelle
385
Fork
42
Merge medio
10h 4m
PR unite (30g)
361

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di getsentry/junior

Tutte le issue di getsentry/junior

Issue simili

Altre issue su TypeScript

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.