Support MSC4446: Move fully read markers backwards

[SpiritCroc]

Upstream PR: element-hq/synapse#19663

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

New Features

• Added experimental feature flag msc4446 to optionally enable backward movement of the fully read marker in conversations.
• New optional com.beeper.allow_backward request parameter available in both read marker and receipt API endpoints when the experimental feature is enabled.
• Feature flag exposed in server versions endpoint to allow clients to discover feature availability and compatibility.

Walkthrough

This change implements a new experimental feature (msc4446) that enables backward movement of fully read markers. A feature flag is added to configuration, passed through REST endpoints to the read marker handler, and advertised in the versions endpoint. The handler gains conditional logic to skip backward-movement restrictions when the feature is enabled.

Changes

Cohort / File(s)	Summary
Configuration & Feature Discovery synapse/config/experimental.py, synapse/rest/client/versions.py	Added experimental feature flag msc4446_enabled (default False) to config parsing, and exposed it in unstable features response.
Handler Logic synapse/handlers/read_marker.py	Added optional allow_backward: bool = False parameter to received_client_read_marker. When True, skips the backward-movement check that normally restricts updates to forward-only events.
REST Endpoints synapse/rest/client/read_marker.py, synapse/rest/client/receipts.py	Added handling for com.beeper.allow_backward request field when feature is enabled. Validates it as a boolean, enforces it only for FULLY_READ receipt types, and forwards to handler.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: adding support for MSC4446 to allow moving fully read markers backwards, which aligns with the changeset modifications across configuration, handler, and REST endpoint files.
Description check	✅ Passed	The description references an upstream PR related to MSC4446 (fully read markers), which is directly relevant to the changeset that implements this feature.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch tobias/msc4446

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

synapse/rest/client/read_marker.py (1)

78-88: Inconsistent validation with /receipt endpoint.

In receipts.py (lines 88-94), allow_backward=true is explicitly rejected for non-FULLY_READ receipt types. Here, if a client sends com.beeper.allow_backward: true without including m.fully_read in the body, the parameter is silently ignored.

Consider adding validation to reject the request when allow_backward is true but no m.fully_read key is present, for API consistency:

Suggested validation

         if self.config.experimental.msc4446_enabled:
             allow_backward = body.get("com.beeper.allow_backward", False)
             if not isinstance(allow_backward, bool):
                 raise SynapseError(
                     400,
                     "com.beeper.allow_backward must be a boolean.",
                     Codes.INVALID_PARAM,
                 )
+            if allow_backward and ReceiptTypes.FULLY_READ not in body:
+                raise SynapseError(
+                    400,
+                    f"com.beeper.allow_backward is only valid when {ReceiptTypes.FULLY_READ} is present.",
+                    Codes.INVALID_PARAM,
+                )
             unrecognized_types -= {"com.beeper.allow_backward"}
         else:
             allow_backward = False

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@synapse/rest/client/read_marker.py` around lines 78 - 88, The current branch
in read_marker.py accepts com.beeper.allow_backward=true and then silently
ignores it if no m.fully_read is provided; mirror receipts.py behavior by
validating this: after parsing allow_backward in the block guarded by
self.config.experimental.msc4446_enabled, if allow_backward is True and the
request body does not contain the "m.fully_read" key, raise a SynapseError(400,
"com.beeper.allow_backward may only be true when m.fully_read is present.",
Codes.INVALID_PARAM) so the invalid combination is rejected; update any related
unrecognized_types handling (the existing unrecognized_types -=
{"com.beeper.allow_backward"}) as needed.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@synapse/rest/client/read_marker.py`:
- Around line 78-88: The current branch in read_marker.py accepts
com.beeper.allow_backward=true and then silently ignores it if no m.fully_read
is provided; mirror receipts.py behavior by validating this: after parsing
allow_backward in the block guarded by self.config.experimental.msc4446_enabled,
if allow_backward is True and the request body does not contain the
"m.fully_read" key, raise a SynapseError(400, "com.beeper.allow_backward may
only be true when m.fully_read is present.", Codes.INVALID_PARAM) so the invalid
combination is rejected; update any related unrecognized_types handling (the
existing unrecognized_types -= {"com.beeper.allow_backward"}) as needed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 988c8a8e-44c4-4383-98de-ae5c1c134fea

📥 Commits

Reviewing files that changed from the base of the PR and between 5ef5d4d and 39fa84f.

📒 Files selected for processing (5)

• synapse/config/experimental.py
• synapse/handlers/read_marker.py
• synapse/rest/client/read_marker.py
• synapse/rest/client/receipts.py
• synapse/rest/client/versions.py

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

• GitHub Check: test-sytest
• GitHub Check: test-trial
• GitHub Check: test-complement
• GitHub Check: lint-types

🔇 Additional comments (7)

synapse/config/experimental.py (1)

603-605: LGTM!

The experimental feature flag follows the established pattern for MSC configuration.

synapse/rest/client/versions.py (1)

183-184: LGTM!

The unstable feature advertisement follows the existing pattern and uses the appropriate namespace.

synapse/rest/client/read_marker.py (1)

105-112: LGTM!

The allow_backward parameter is correctly passed only for FULLY_READ receipts, matching the handler's expected behavior.

synapse/rest/client/receipts.py (2)

79-96: LGTM!

Thorough validation ensuring allow_backward is properly typed and restricted to FULLY_READ receipts only.

---

128-135: LGTM!

The allow_backward parameter is correctly forwarded to the read marker handler for FULLY_READ receipts.

synapse/handlers/read_marker.py (2)

42-49: LGTM!

The new allow_backward parameter with a safe default of False maintains backward compatibility.

---

66-77: LGTM!

The conditional logic correctly skips the ordering comparison when allow_backward=True, allowing the marker to move backwards as specified by MSC4446.

[SpiritCroc]

I suppose this would be how we enable it after merging this: https://github.com/beeper/stack/pull/211