Resolve actor identity once and store it explicitly

Aperta
#2,439 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
20/100
Tipo di issue
Refactoring
Chiarezza
Abbastanza chiara
Stato di attività
Ferma
Stack tecnologico
github, redis, typescript

Direzione di ricerca

Start with the merged spec in openspec/changes/actor-identity-end-state/ and its design.md; chat/actor.ts is the runtime Actor entry point named in the issue. This issue coordinates several independently shipped steps, so choose a specific step and read its dependency issues before starting. Done means that step meets the spec and release rules; implementation details and tests are settled in its PR.

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

Descrizione

Junior's runtime Actor is typed (chat/actor.ts), but stored Conversation state flattens it to a Slack-only shape. Later write and read paths then guess the Identity again from platform strings, the Source, the Location, and id patterns. Each guess can drift, and they caused #2431 and its follow-up audit.

Current state

  • Conversations and Redis work state store a Slack-only Actor. Web Actors lose their kind. System and local Actors are left out.
  • #2443 merged the Actor-to-Identity mapping into one entry point. #2473 added the readers for the Identity id (released in 0.268.0). #2482 added the writers (merged, not released yet).
  • In released code (0.268.0), the Conversation write lets the newest Identity win, so a human Turn can replace a system root's Identity. This is the root cause of #2431. #2436 works around one case. #2482 (merged) makes the write keep the first value. Until a release with #2482 runs on every worker, the damage can keep happening.
  • The Conversation Identity column (junior_conversations.actor_identity_id) does four jobs: the root that the dashboard shows, private access, spend, and the personal feed. Child rows store their own copy, and spend reads it. Two other Identity columns are always empty.
  • No inbound event Message has an author Identity.
  • Session logs, activity metadata, Guardian, and root Conversation Message authors also guess the Identity.

End state

The source of truth is the merged spec on main: openspec/changes/actor-identity-end-state/ (#2444, updated by #2489). It matches the plan below. It fixes the target and the rules. Each step settles its own implementation details in its PR, after it re-checks the spec against current main. A change to a decided rule needs David's OK.

  • Owner: the Identity that started the root Conversation: the human for Slack, web, and fork; the one system Identity for automations, plugin dispatch, and the local CLI. Set once at insert. It never transfers. An Automation has a creator, not an Owner.
  • The Owner column is the existing junior_conversations.actor_identity_id. Code and docs call it the Owner. No new column and no rename.
  • Children store no Owner. They read the root Owner, and child spend rolls up to it.
  • One system Identity, with the fixed id system (#2482). "Which system" is provenance (Source and #2211). Dashboard labels come from the Source and do not change to "System".
  • Users have many Identities. Spend, People, participants, and Locations roll up by User.
  • Web users use their sign-in Identity. The mock viewer has a mock Identity. Junior stops writing junior/<email>.
  • Bots are kind bot with no User.
  • Every new user Message has an author Identity. Assistant output has none.
  • Events are user Messages. The author is the person or bot who wrote the content, or the system Identity for a system event. Event authors are not participants.
  • The event author is never the Turn Actor. Event Turns stay system-acted, and credentials come only from the Watch or Automation grant. design.md explains the confused deputy risk. Junior still resolves the author to its Identity and linked User, for the Message label and the trust check.
  • Trust: an event author is trusted when Junior verifies them on an allowed domain (the #2260 / #2217 check) or their login is on trustedAuthors. Trusted text counts like a chat message. Untrusted or unknown authors do not run. Plan reading: Junior drops their events before batching, so the agent never sees them. System events run as today, with the current untrusted-text handling.
  • Legacy data rule (plan decision): migrate an old data shape only when special code exists because of it, and only with a rule that a read-only dry run proves. Then delete that code.
    • Needs migration: the stored Actor JSON (actor_json), user Messages with no author Identity, the payload copy of the author id, the per-name scheduler / local-cli / event system rows (merge into system, after the labels come from the Source), Owner copies on child Conversations, and the two unused columns creator_identity_id / credential_subject_identity_id.
    • No migration: old junior/<email> web Identities (valid Identities linked to the right User; a merge into the sign-in Identity is optional), and Redis and Turn checkpoint data (Junior waits for it to expire: 7-day Redis TTL; the plan waits 14 days).
    • Old event Messages get the system Identity as author. Plan reading of David's answer 2: old user Messages whose author cannot be proven stay with no author, with no guess. A human Message stored as system could later read as a system or automation Message in history.
  • Owner repair (#2431) is separate (plan decision): it fixes wrong data, not a code path. A root with a system Source (scheduler, event, local) that a human took over goes back to system; this is provable because the Source is set-once. A Slack or web root with a system Owner is repaired only if the dry run proves the rule. Humans who lose a root keep access as participants.

Decided by David Cramer in the Slack thread

  • The Owner is the person who started the Conversation, or the system for an automation. Owners never transfer.
  • There should be no unknown actors on any Message from now on.
  • The local CLI user can be the system Identity.
  • A web user is the User they sign in with, through the normal User and Identity tables.
  • The schema should be normalized.
  • Assistant output is just an event. It needs no author Identity.
  • Web users always have an Identity. The mock viewer mocks one.
  • Child spend rolls up to the root Owner.
  • Slack bots have an Identity but no User.
  • Inbound event authors: Junior finds the correct Identity and verifies it the same way as everywhere else. Junior ignores the instructions of someone without access.
  • Most events are system events, with the system Identity as author. PR feedback and comments are authored by the person or bot who wrote them.
  • Legacy junior/<email> rows: leave them as they are, unless a migration path is clearly correct.
  • Participants: "same as today: users." Verified event authors as participants: "not right now maybe we will change our mind later."
  • "also we'd allow list usernames in juniors config for github events (so we could allow list some bots)"
  • "call it trusted authors or sometihng", then: "yes "trusted" is what we want here, but you can use the suffix that makes sense"
  • "any sender who doesnt have the allowed domain email address or is unknown isnt trusted. bots and or other users could be listed under trusted"
  • On what trusted text may do: "trusted authors are the same as normal authors... its just that untrusted authors we dont run"
  • On the event author as Turn Actor: "I agree, lets not have it be the turn actor as it expands the security concerns. you need to document that (confused deputy concerns for example). knowing who it is that maps to our internal users is valuable tho so keep that"
  • On splitting batches by trust and hiding untrusted entries: "untrusted events wont actually ever be seen so i think (2/3) is not relevant? system events might have untrusted text but we already handle that"
  • On Cursor Bugbot: "yeah Bugbot and sentry will end up on trusted actors"
  • His answers to the 14 proposals in #2444: (1) bot kind: "bots need to be annotated as bots, thats important"; (2) bypass Identity: "you can find what makes sense"; (3) doneBy fields: "yes"; (4) upsert keeps kind and user: "correct"; (5) events: "yes but an event is a message so"; (6) case-insensitive match: "yes"; (7) Owner repair: "sure defer to you but needs to be correct"; (8) Locations by User: "yep"; (9) author shape: "yep. and users have many identities associated to them"; (10) system key: "thats meaningless to me, make sure its good arch and ill trust you"; (11) Slack bot subject: "yep"; (12) mock key: "idk up to you make it correct"; (13) Automation creator: "sure, we dont really have "owners" on automations anyways"; (14) instruction record: "ok".
  • His answers on old data and the Owner column: (1) old event Messages get the system Identity as author: "(1) yes"; (2) old user Messages whose author cannot be proven: "(2), why not just make them system? if they dont have an author just make them without an author? should be uncommon"; (3) keep junior_conversations.actor_identity_id as the Owner column: "(3) i guess? usually theres just like one actor.. everything else is metadata".

Handed off by David (2, 7, 10, 12): each is a current proposal in the spec that its step confirms. The Owner repair runs only for the rules that the dry run proves, after David's OK.

Resolved questions

  • Mixed event batch (drop untrusted entries, or keep them as context): resolved by his "untrusted events wont actually ever be seen" answer. Plan reading: Junior drops untrusted authored events before batching. Only trusted and system entries are batched.
  • Cursor Bugbot on trustedAuthors: resolved by his "Bugbot and sentry" answer. Plan reading: he wrote "trusted actors", and the plan reads this as the GitHub plugin trustedAuthors list. Cursor Bugbot and Sentry's GitHub bot go on that list in the deployed app config. Step 4 confirms the exact logins from real event senders. That config ships with the trust rule, or their PR feedback stops starting runs.
  • Old data and the Owner column: resolved by his three answers above. Plan decision: the new Owner column work (old steps 4a/4b) is removed, because #2482 makes the existing column keep its first value, which is the Owner rule.

No open questions remain.

Steps (each ships on its own)
Next: step 3.

  1. One Actor-to-Identity mapping: #2443 (merged).
  2. Carry the Identity id from ingress. Readers: #2473 (merged). Writers: #2482 (merged, not released yet). The step 8 Redis window starts when a release with #2482 runs on every worker.
  3. System root labels come from the Source, new system Conversations use system, code calls the column the Owner, every reader uses the root Owner (including child spend and Locations by User), and children stop storing an Owner. Readers ship before children stop storing it.
  4. Real Identities and trust: web uses the sign-in Identity, mock and bypass Identities, bots get the bot kind, event authors, the trust rule, and trustedAuthors with Bugbot and Sentry. Needs #2260.
  5. Read-only dry run of every migration and repair rule. Post the counts on #2439, then David's OK.
  6. One data migration release, only for the rules the dry run proved.
  7. Owner repair (#2431), only for the rules the dry run proved.
  8. After the Redis window (14 days after step 2 runs on every worker): delete the fallbacks, require an author on new user Messages, and remove the stored Slack-shaped Actor from Conversations and Redis. Also the Guardian, session log, and dashboard author shape changes. Needs #2181.
  9. One release later: drop the old columns, move the durable rules to the owning READMEs, and delete the spec.

Release rules: readers before writers, safe under rolling deploys, column drops one release after the code stops using them, no production backfill without explicit approval, and a dry run before any migration or repair.

Dependencies: #2260 before 4, #2181 before 8. #2211 owns which Automation or dispatch did the work.

Not covered: keying user credentials by User, the closed Actor schema, and naming the local CLI human. Visibility rows written wrong before #2440 belong to #2382. Known gaps are listed in design.md.

Related: #2260, #2431, #2432, #2436, #2437, #2440, #2443, #2444, #2473, #2482, #2489.

via David Cramer.

--

View 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.