Skip to content
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

MSC4253: Modifying or rejecting accepted MSCs #4253

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

MSC4253: Modifying or rejecting accepted MSCs #4253

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
972bf8b
MSC: Modifying or rejecting accepted MSCs
turt2live Jan 15, 2025
367c0e9
Apply suggestions from code review
turt2live Jan 15, 2025
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
65 changes: 65 additions & 0 deletions proposals/4253-spec-process-modify-reject-accepted.md
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

Implementation requirements:

None

Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# MSC4253: Modifying or rejecting accepted MSCs

The [spec process](https://spec.matrix.org/proposals/) defines the stages and steps an MSC goes through
before becoming actual specification. That process has an awkward accepted-but-not-merged state where
the MSC has successfully completed a *merge* FCP, but has not yet had a spec PR merged to introduce
that MSC to the formal specification. During this time, implementations MAY use stable identifiers when
speaking the MSC.

The general theory of this stage is that after several rounds of review, the MSC is unlikely to materially
change, but the MSC *could* still change if needed. Typically, this would be most likely to happen
during spec PR review.

The spec process does carve out the awkward state for potential modification, but does not describe
how modification (or post-acceptance rejection) actually happens though. This proposal clarifies what
is believed to be current operating procedure for these two actions.

## Proposal

For MSCs which are *accepted* but not *merged* (anywhere between `finished-final-comment-period` with
`disposition-merge` to `spec-pr-in-review`, inclusive), the following process steps MAY be taken:

1. The SCT may choose to revert FCP acceptance to bring the MSC back to "open, in review" or pull the
MSC to `rejected` instead. The SCT is required to provide guidance to stable implementations, if
applicable, for how to handle the change when this happens. It is left as an implementation detail
to determine whether an MSC is brought back to `in-review` or pushed to `rejected`, and how to
enact this process step. This step is known as "post-acceptance rejection", regardless of target
state for the affected MSC.

2. Another MSC is required to change the text of the accepted MSC, provided it does two things:

1. Describe the rationale for the change being made in a dedicated MSC; and
2. Modify the accepted MSC's actual text in the same GitHub PR. This is to ensure that the change
is captured in two ways: with a dedicated, unmerged, MSC and as real rendered text in the event
someone is reviewing the accepted MSC's text.

When reviewing spec PRs for accuracy to their respective MSCs, reviewers are encouraged to use the
accepted, rendered, text as reference rather than the original GitHub PR for the MSC due to PRs not
updating post-merge.
Comment on lines +37 to +38
Copy link
Member

Choose a reason for hiding this comment

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

Maybe clarify this means "what's in the main branch"?

Comment on lines +22 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would the mess of modifying an accepted MSC through another be preferable over reopening/replacing the original?

I'm not even sure reopening one is great when that does not really fit the tooling (i.e. GitHub workflow/limitations), when it can be replace with a new one for free?

Copy link
Member Author

Choose a reason for hiding this comment

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

Github doesn't allow a merged PR to be reopened. Process-wise, we'd end up always rejecting and rewriting if we were to do that, which is high process for little value compared to just writing a small diff.


## Potential issues

Spec PR reviewers may still miss MSCs which modify another accepted MSC. SCT members should consider
when it is more appropriate to use post-acceptance rejection instead of modification. For example, it
may be more correct to pull an MSC back to `in-review` when a modification would be significant or
easily forgotten.

Stable implementations of accepted MSCs may be severely affected by either process step. The SCT is
expected to work out a plan for how to address those incompatibilities when performing either step.
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly a stupid question but what if using stable identifiers was prohibited until the MSC actually lands in a spec release? That is to say treat it as unstable until it is actually released.

Copy link
Member Author

Choose a reason for hiding this comment

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

It'd probably have no effect, as there's no stable implementations. Room versions would fall under this because they require another MSC to actually make them stable.

Copy link
Member

Choose a reason for hiding this comment

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

This used to be the case but it felt it slowed down ecosystem progress too much by tying features to spec releases. It would simplify handling of version flags though.

Copy link
Member Author

Choose a reason for hiding this comment

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

(oh, I failed at reading the question, sorry - clokep's response is much more accurate)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe with quarterly releases this is no longer necessary, however?

Copy link
Member

Choose a reason for hiding this comment

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

#3179 should hopefully be a launch point for context.

Copy link
Member

@clokep clokep Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oddly that change wasn't by an MSC?

Copy link
Member Author

Choose a reason for hiding this comment

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

Process changes aren't required to have an MSC, but it can be helpful depending on the level of feedback someone is after. In that case, the SCT spent significant time discussing it internally before making the PR, so felt an MSC was not required.

Copy link
Member Author

Choose a reason for hiding this comment

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

on the actual topic of stable identifiers: the spec process is supposed to already acknowledge that while developers can switch to stable identifiers, there is still risk in doing so. Developers also can't assume what version of the spec a feature will land in, which makes some/many stable identifier usages less useful without the "unstable stable flag" on /versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe with quarterly releases this is no longer necessary, however?

Yeah, it doesn't seem like an extensive benefit anymore.

It also feels a little odd to me to implement something that is not spec'ed as if it was spec'ed, especially when RFC keywords are often not explicitly nailed down in MSCs.


## Alternatives

No significant alternatives.

## Security considerations

No significant concerns.

## Unstable prefix

No unstable prefix is required for process MSCs.

## Dependencies

No direct dependencies, though [MSC4252](https://github.com/matrix-org/matrix-spec-proposals/pull/4252)
makes use of this MSC.
Loading