don't try to reassign a reviewer when a pr title is edited

[jyn514]

doing so leads to weird errors:

Could not assign reviewer from: jyn514. User(s) jyn514 are either the PR author, already assigned, or on vacation, and there are no other candidates. Use r? to specify someone else to assign.

this was likely missed when transitioning from highfive to triagebot.

see https://github.com/rust-lang/rust/pull/118993 for an example of a bug this fixes.

[ehuss]

I don't think this is the correct thing to change. We want this to handle Edited comments. For example, if someone writes a comment like r? wrong-person, and then they edit the comment to r? correct-person, we want it to reassign to the correct user (not handling that has confused people in the past).

I'm not entirely sure what happened to cause #1750 to happen. Just changing the title normally doesn't cause it to attempt to reassign. There are multiple guards to prevent this from happening:

• Attempting to reassign to someone already assigned should be ignored here.
• Editing a comment where there is no change should not re-issue the command here.

I'm not able to reproduce the problem by just editing a PR title. Can you maybe dig into the exact sequence of events that causes this to happen? It seems like the primary clues are that the PR comment includes r? username, and that the user edits the title sometime aftewards. However, in my testing, that alone is not sufficient to trigger the issue.

My initial thoughts were that it was maybe a race condition with overlapping webhooks for opening the issue and the subsequent title rename. However, it looks like https://github.com/rust-lang/rust/pull/117522#issuecomment-1817484074 happened weeks afterwards.

[ehuss]

BTW, thanks for taking a look at fixing this!

[jyn514]

We want this to handle Edited comments. For example, if someone writes a comment like r? wrong-person, and then they edit the comment to r? correct-person, we want it to reassign to the correct user (not handling that has confused people in the past).

Github sends a different event when comments are edited than when the PR is edited. Here is the action it sends for a comment: https://github.com/rust-lang/triagebot/blob/fa84cebec4b34c904f1e4cf784359f8870bdfe38/src/github.rs#L920 and for a PR: https://github.com/rust-lang/triagebot/blob/fa84cebec4b34c904f1e4cf784359f8870bdfe38/src/github.rs#L937

So this is only preventing people from editing the PR description to send triagebot commands, not editing comments. I'll talk in a second about why I think it would be hard to support that.

Attempting to reassign to someone already assigned should be ignored here.

That code is in set_assignee. But the error on the PR comes from AllReviewersFiltered, which is returned by find_reviewer_from_names, which is called before set_assignee: https://github.com/rust-lang/triagebot/blob/c156fe97441daa5c7fb0644a8e5a036511a37726/src/handlers/assign.rs#L511-L522

Editing a comment where there is no change should not re-issue the command here.

The previous text there comes from issues_event.changes.body.from: https://github.com/rust-lang/triagebot/blob/fa84cebec4b34c904f1e4cf784359f8870bdfe38/src/github.rs#L1847 github documents that body text as: https://docs.github.com/en/rest/using-the-rest-api/github-event-types?apiVersion=2022-11-28#event-payload-object-for-issuesevent

changes[body][from] The previous version of the body if the action was edited.

I suspect that somehow we are deserializing the wrong part of the JSON object somehow and previous ends up as None. Unfortunately, we have no debug logging for this part of the code so we can't know for sure. Another possibility is that Github is only sending us the title when the title is changed, and not the body, because the body is the same as before.

---

I'm not able to reproduce the problem by just editing a PR title. Can you maybe dig into the exact sequence of events that causes this to happen? It seems like the primary clues are that the PR comment includes r? username, and that the user edits the title sometime aftewards. However, in my testing, that alone is not sufficient to trigger the issue.

do you have a repo where i can play around with triagebot without sending people notifications? i'm not willing to try and run my own instance locally.

[jyn514]

I'm not able to reproduce the problem by just editing a PR title. Can you maybe dig into the exact sequence of events that causes this to happen? It seems like the primary clues are that the PR comment includes r? username, and that the user edits the title sometime aftewards. However, in my testing, that alone is not sufficient to trigger the issue.

say more about how you tested this? i tried this out on a test repo and was able to reproduce it every time i edited the pr title: https://github.com/WaffleLapkin/teloxidebottest/pull/8

[jyn514]

ok cool i think i know what's going on.

here's some logging from https://github.com/rust-lang/triagebot/pull/1755/commits/bf58902754a224b144d37b5c70008e7bb572ec1a deployed onto https://github.com/WaffleLapkin/teloxidebottest/pull/9:

``` Dec 17 03:14:28 arch-vps triagebot[3081786]: 2023-12-17T03:14:28.084755Z INFO request{uuid=1b1cb217-b039-4905-b011-a61a26e1b561}: triagebot: handling issue event IssuesEvent { action: Opened, issue: Issue { number: 9, body: "fixes #7\r\n\r\nr? WaffleLapkin", created_at: 2023-12-17T03:14:26Z, updated_at: 2023-12-17T03:14:26Z, merge_commit_sha: None, title: "document more important waffle facts", html_url: "https://github.com/WaffleLapkin/teloxidebottest/pull/9", user: User { login: "jyn514", id: Some(23638587) }, labels: [], assignees: [], pull_request: Some(PullRequestDetails { files_changed: OnceCell { value: None } }), merged: false, draft: false, comments_url: "https://api.github.com/repos/WaffleLapkin/teloxidebottest/issues/9/comments", repository: OnceCell(Uninit), base: Some(CommitBase { sha: "4ce06a75c0812e3e471c683ee63eba715533f068", git_ref: "master", repo: Repository { full_name: "WaffleLapkin/teloxidebottest", default_branch: "master", fork: false, parent: None } }), head: Some(CommitBase { sha: "94ce8cc13474b5c61b59e1453a5ebf3892807d48", git_ref: "waffle-is-adorable", repo: Repository { full_name: "WaffleLapkin/teloxidebottest", default_branch: "master", fork: false, parent: None } }), state: Open }, changes: None, repository: Repository { full_name: "WaffleLapkin/teloxidebottest", default_branch: "master", fork: false, parent: None }, sender: User { login: "jyn514", id: Some(23638587) } } ```

notice in particular this bit:

handling issue event IssuesEvent { action: Edited, changes: Some(Changes { title: Some(ChangeInner { from: "document more important waffle facts" }), body: None }) } 

that explains why the second check is not firing: we are not getting a changes[body] object, so we don't have the previous text to compare it to.

---

you mentioned wanting to support editing (as mentioned above i am talking specifically about the case of editing the PR description, not a later comment). i think that will be error-prone and would prefer to disallow it, but if we were going to do it, i would suggest one or more of the following approaches:

1. unconditionally ignoring commands in the PR description if changes[body] is None
2. moving the check for reassigning to a currently-assigned reviewer earlier, from set_assignee to find_reviewer_from_names. this will be tricky because find_reviewer_from_names is used in several different contexts.
3. still keeping my change here which avoids handling this in handle_command, but changing handle_input to only return early if there's an assignee on the Opened event, not the Edited event.

[ehuss]

Thanks for investigating! I think I understand it better now.

unconditionally ignoring commands in the PR description if changes[body] is None

This sounds like the right solution to me. That is, ignore if the action is Edited and the changes body is None.

[WaffleLapkin]

btw, just saw another case of the bug: https://github.com/rust-lang/rust/pull/118391#issuecomment-1861151297