Skip to content

fcp merge: allow all known users to start team-specific FCPs #352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Mark-Simulacrum merged 1 commit into from
Jun 6, 2026
Merged

fcp merge: allow all known users to start team-specific FCPs #352

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/github/nag.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,16 +76,20 @@ pub fn update_nags(comment: &IssueComment) -> DashResult<()> {

any = true;

if let RfcBotCommand::StartPoll { .. } = command {
// Accept poll requests from any known user.
if let RfcBotCommand::StartPoll { .. }
| RfcBotCommand::FcpPropose(FcpDispositionData::Merge(Some(_))) = command
{
// Accept poll requests and "fcp merge team" from any known user.
if all_team_members.iter().find(|&u| u == &author).is_none() {
// FIXME: post an error message in the issue.
info!("poll requester ({}) is not a known user", author.login);
return Ok(());

@RalfJung RalfJung May 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it enough to just return Err(DashError(CommentableError(...))) here to get a proper error message?

@Mark-Simulacrum Mark-Simulacrum Jun 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so -- not entirely sure, but my guess is that the right thing is to leave a comment inline here via:

                let comment = RfcBotComment::new(&issue, CommentType::Error { message });
                comment.post(None)?;

But it's not entirely obvious to me how that gets de-duped when rfcbot re-polls comments for additional processing. Maybe that's handled above this function (I sort of hope so...).

I'm personally OK merging without it, IIUC we don't currently return a comment error saying we failed to run the fcp because the person didn't have permissions to do so?

@RalfJung RalfJung Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't currently return a comment error saying we failed to run the fcp because the person didn't have permissions to do so?

Yeah we don't, and that has led to a lot of confusion over the years because the bot regularly thinks people don't have permission when they really should. This is the underlying cause of #312.

But indeed we don't have to fix that in this PR.

}
} else {
// Don't accept most bot commands from non-subteam members.
// Early return because we'll just get here again...
if subteam_members.iter().find(|&u| u == &author).is_none() {
// FIXME: post an error message in the issue.
info!(
"command author ({}) doesn't appear in any relevant subteams",
author.login
Expand Down