Repository navigation
feat(events): Treat linked requester PR comments as their instruction - #2260
Open
sentry-junior[bot] wants to merge 4 commits into
Open
sentry-junior[bot] wants to merge 4 commits into
sentry-junior[bot] wants to merge 4 commits into
Conversation
A Watch now stores the Slack user whose own message created it. When a GitHub comment or review comes from the immutable GitHub account that links to that same Slack user, core adds the author's own text (without quotes) as a requester instruction. The agent and Guardian treat it as a message from that person. Every other comment and the PR body stay untrusted, and the Event Turn keeps the system Actor and its credentials. Refs #2258 Co-Authored-By: BYK <burak.kaya@sentry.io>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dcramer
marked this pull request as ready for review
October 28, 2026 00:38
…hority # Conflicts: # packages/junior-github/src/webhooks/events.ts # packages/junior-github/tests/webhook-outcomes.test.ts # packages/junior/src/chat/events/notification.ts # packages/junior/src/chat/guardian/gate.ts # packages/junior/src/chat/guardian/reviewer.ts # packages/junior/src/chat/task-execution/conversation-turn.ts Co-Authored-By: BYK <burak.kaya@sentry.io>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Quotes inside lists and HTML blockquote tags stayed in doneByText, so text that only quotes someone else could count as a requester instruction. Walk the Markdown tokens at every depth, and drop HTML quote blocks until they close.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a4ef59. Configure here.
The HTML quote tracker counted closing tags anywhere, also in HTML comments and attributes. A fake closing tag ended the quote early, so quoted text could become a requester instruction. Do not trust closing tags: remove the block with the first HTML quote tag and all text after it.
sentry-junior Bot
added a commit
that referenced
this pull request
Oct 30, 2026
A cold review found gaps that break a rolling deploy or widen trust. - Split the Owner step. 4a writes the column, and 4b adds the NOT VALID check one release later, because old workers insert roots with no Owner while migrations run. - Drop each column in two releases. conversationReadColumns() selects every schema column, so release N stops reads and writes and N+1 drops it. Add an explicit stop of actor_json writes. - Pin Guardian labels to the Actor and provenance, never the one system Identity id, and keep trusted event text in the Requester instruction section. - Use findLinkedSlackMember() for event author trust, the same check as #2217 and #2260. - Pin Identity keys, the mailbox field, and the full system merge. - Drop hasActor and list the remaining choices as proposals. Refs #2439 Co-Authored-By: David Cramer <david@sentry.io>
dcramer
added a commit
that referenced
this pull request
Oct 31, 2026
Refs #2439 This adds the plan for #2439 in `openspec/changes/actor-identity-end-state/`. It also adds **Owner** and **Trusted author** to `TERMINOLOGY.md`, and says that an Automation has a creator, not an owner (also in the dashboard Automation editor README). There is no code change. The plan fixes the target and the rules. It does not fix the implementation. At the start of each step, the step re-checks the plan against current `main` and settles its own details (SQL, keys, field names, signatures, readers, tests) in its PR. A change to a decided rule needs David's OK. - `proposal.md`: scope, David's answers, plan decisions, open questions, behavior changes, and risks. - `design.md`: the model and rules, the current proposals that a step must confirm, and the known gaps. - `tasks.md`: how to use the plan, release rules, dependencies, and steps 1–7. Each step has a goal, a done outcome, constraints, and what to decide in that step. - `specs/conversation-identity/spec.md`: the end-state requirements and scenarios. **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. - For an inbound event, 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" His answers to the 14 earlier proposals: (1) "bots need to be annotated as bots, thats important"; (2) "you can find what makes sense"; (3) "yes"; (4) "correct"; (5) "yes but an event *is* a message so"; (6) "yes"; (7) "sure defer to you but needs to be correct"; (8) "yep"; (9) "yep. and users have many identities associated to them"; (10) "thats meaningless to me, make sure its good arch and ill trust you"; (11) "yep"; (12) "idk up to you make it correct"; (13) "sure, we dont really have "owners" on automations anyways"; (14) "ok". **Plan decisions David handed off (2, 7, 10, 12).** Each is a current proposal that its step confirms: the local auth bypass gets an Identity that cannot match a production User (step 3); the Owner repair runs only when it is clearly correct, after a dry run (step 4b); one system Identity with a key that no plugin or OAuth link can write (#2443); the mock viewer Identity is in memory only (step 3). **Target, from those answers:** one Owner per root, set at insert, human or system, never transferred. Children read the root Owner, and child spend rolls up to it. One system Identity; "which system" is provenance. Web users use their sign-in Identity; the mock viewer has a mock Identity. Bots are kind `bot` with no User. Every new user Message has an author Identity; assistant output has none. Events are user Messages and only set the Message author. An event author is trusted when Junior verifies them on an allowed domain or their login is on `trustedAuthors`; untrusted or unknown authors do not run. Event authors are not participants. Legacy rows stay unless a migration is clearly correct. **Release rules:** each step ships alone and is safe under a rolling deploy. Readers ship before writers. A column drop ships one release after the code stops using it. No production backfill without explicit approval, and a dry run before any repair. **Steps:** (1) one Actor-to-Identity mapping, built in #2443 (in review); (2) carry the Identity id; (3) real Identities and the trust rule; (4a) write the Owner; (4b) backfill, enforce, and read the Owner (riskiest); (5) require authors and retire stored Actors; (6) stop using the old columns; (7) drop them. #2441 before 2, #2260 before 3, #2382 before 4a, #2181 before 5. **Open questions for David:** 1. Mixed event batch: are untrusted entries dropped, or kept as context that does not start a run? Plan reading: dropped. 2. Does Cursor Bugbot go on `trustedAuthors`? Plan reading: not listed yet, so its reviews stop starting runs when step 3 ships. **Behavior changes:** automation-run spend leaves personal spend. A human who replies to an automation Conversation becomes a participant, not the Owner. Child spend goes to the root Owner. Trusted author text counts like a chat message; untrusted or unknown authors do not run. The participant list stops showing system Identities. Locations show one row per User. <!-- junior-request-attribution:start --> via **David Cramer**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3Af31d0b3589d3ee280ff739bacb901c33 --> -- [View Session](https://junior-prod.sentry.dev/conversations/agent%3Af31d0b3589d3ee280ff739bacb901c33) [[Sentry]](https://sentry.sentry.io/explore/conversations/agent%3Af31d0b3589d3ee280ff739bacb901c33/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: David Cramer <david@sentry.io>
dcramer
added a commit
that referenced
this pull request
Oct 31, 2026
… legacy data (#2489) Refs #2439 Updates the Actor identity plan (`openspec/changes/actor-identity-end-state/`) to the reevaluated plan that David answered in the #2439 Slack thread. David's answers, quoted in `proposal.md`: - "(1) yes": old event Messages get the system Identity as their author. - "(2), why not just make them system? if they dont have an author just make them without an author? should be uncommon": plan reading: old user Messages whose author cannot be proven stay with no author. The dry run counts them. - "(3) i guess? usually theres just like one actor.. everything else is metadata": `junior_conversations.actor_identity_id` stays as the Owner column. Code and docs call it the Owner. Plan decisions: - The new Owner column work (old steps 4a/4b) is gone. #2482 makes the existing column keep its first value, which is the Owner rule. - New legacy data rule: migrate an old shape only when special code exists for it, and only with a rule that a read-only dry run proves. Then delete that code. `design.md`, Legacy Data, lists the shapes that need it and the ones that do not (`junior/<email>` Identities, Redis and Turn checkpoints). - Owner repair (#2431) is a separate step. It fixes wrong data, not a code path. - The Bugbot open question is closed with David's "yeah Bugbot and sentry will end up on trusted actors". New step order: 1 (#2443, done), 2 (#2473 done, finish #2482), 3 labels, `system` for new system Conversations, child Owner, and root Owner readers, 4 real Identities and trust (needs #2260), 5 dry run and David's OK, 6 one data migration, 7 Owner repair, 8 delete fallbacks and stored Actors after the Redis window, 9 drop columns and delete the plan. Steps 1 and 2 keep their numbers, so the references in #2482 stay correct. All other decided rules stay the same. `TERMINOLOGY.md` does not change. <!-- junior-request-attribution:start --> via **David Cramer**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3A792f6b2d81b457a9b23a84193e893814 --> -- [View Session](https://junior-prod.sentry.dev/conversations/agent%3A792f6b2d81b457a9b23a84193e893814) [[Sentry]](https://sentry.sentry.io/explore/conversations/agent%3A792f6b2d81b457a9b23a84193e893814/?project=4510944073809921) <!-- junior-session-footer:end --> Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: David Cramer <david@sentry.io>
This branch was successfully deployed
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.
Closes #2258. Stacked on #2217, which adds the linked Slack identity lookup and membership checks this uses.
When someone asks Junior in Slack for a PR and Junior watches it, a follow-up comment from that same person on the PR is now treated as their instruction for that Event Turn. Before, every PR comment was untrusted, so Guardian blocked things like "also file a docs issue" even when the requester asked for it.
How the match works (fails closed at each step):
doneBy.subjectId) and the author's own text (doneByText) for comments and reviews. Markdown quote lines are removed, since quoting is not the author's own words. Logins never match.findLinkedSlackMember()check as Identity dispatch: one exact linked account, an active human member, an allowed domain. The linked user must also be the requester of that Watch in the same workspace. The check is now shared with feat(dispatch): Start DM dispatches for linked provider identities #2217's dispatch code instead of copied.Requester instructionsection in the task input. Guardian gets it in its own section with a random key, and only for an Event Source. It is saved on the Turn's Message, so a resumed Run keeps it.What does not change: the Turn still runs as the system Actor with the same credentials. Visibility and credential authority do not widen. Other people's comments, quoted text, and the PR body stay untrusted. If the lookup fails, the Event is delivered without the instruction. Child agents do not get requester instructions yet.
New Guardian evals in
evals/guardian/requester-comments.eval.tscover three cases: requester asks for a cross-repo issue (allow), a different reviewer asks for the same thing (ask/deny), and a comment with forged requester text (deny). I could not run them locally. They need CI.via BYK.
--
View Junior Session in Sentry