1.157.2 - #63
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| "com.beeper.user_id": self.user_id, | ||
| "prio": priority, | ||
| } | ||
| if not self.disable_badge_count: |
There was a problem hiding this comment.
Latent regression: com.beeper.server_type dropped when disable_badge_count=True.
Before commit b72880c4c, the initial content dict carried counts = {"unread": badge, "com.beeper.server_type": "synapse"} and was only overwritten to {"unread": badge} in the not disable_badge_count branch. Net effect: when disable_badge_count=True, com.beeper.server_type was sent (kept as part of the default), and when disable_badge_count=False, it was (buggily) stripped.
After the refactor, content no longer has a default counts, so when disable_badge_count=True the entire counts dict is absent and com.beeper.server_type is lost. Any downstream consumer (Sygnal, custom relays) keying off counts.com.beeper.server_type to identify Synapse-originated pushes won't see it for pushers configured with badges disabled.
Since this field is server-identity metadata rather than a count, it likely belongs on content directly. Suggested fix — hoist it out of counts and set it unconditionally:
content["com.beeper.server_type"] = "synapse"
if not self.disable_badge_count:
content["counts"] = {"unread": badge}Apply the same in the full-content branch (~line 515) and _send_badge (~line 551), and add a disable_badge_count=True case to tests/push/test_http.py.
| user_id, room_id, "com.beeper.inbox.done", done | ||
| ) | ||
| logger.info( | ||
| f"SetBeeperDone done_delta_ms={delta_ms} at_order={body.get('at_order')}" |
There was a problem hiding this comment.
This log line still reads body.get('at_order') at the top level, but commit 1b1872e2 moved the field to body["done"]["at_order"]. Result: this log always prints at_order=None, even when the client sent the value. Should match the read on line 356.
| f"SetBeeperDone done_delta_ms={delta_ms} at_order={body.get('at_order')}" | |
| f"SetBeeperDone done_delta_ms={delta_ms} at_order={body['done'].get('at_order')}" |
| body = parse_json_object_from_request(request) | ||
|
|
||
| done = {"updated_ts": ts, "at_ts": ts} | ||
| for room_id in body["room_ids"]: |
There was a problem hiding this comment.
body["room_ids"] is not validated. If a client omits the key, this raises KeyError and Synapse returns 500 instead of a 400. If it's a string, iterating will loop over characters and attempt to write account data for each. The sibling RoomBeeperInboxStateServlet.on_PUT already validates RoomID.is_valid(room_id) — do the same here (plus a presence + list-of-strings check).
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Regex pattern for detecting a bridge bot (cached here for performance) | ||
| SYNAPSE_BOT_PATTERN = re.compile(r"^@_.*_bot\:*") |
There was a problem hiding this comment.
Two loose-anchor problems here:
HUNGRYSERV_BOT_PATTERNdoesn't escape the.inbeeper.local, so it also matches things like@fooxbotxbeeperzlocal.- Neither pattern anchors at the end (
$), and the trailing\:*accepts zero-or-more colons, so@_foo_bot_notreally:example.commatchesSYNAPSE_BOT_PATTERN.
Suggested tightening:
SYNAPSE_BOT_PATTERN = re.compile(r"^@_.*_bot:[^:]+$")
HUNGRYSERV_BOT_PATTERN = re.compile(r"^@[a-z]+bot:beeper\.local$")Silent filtering of user IDs the user tried to add is surprising — worth over-filtering as little as possible.
| """ | ||
| priority = "low" | ||
| priority = "high" # Beep: always use high priority | ||
| if ( |
There was a problem hiding this comment.
This whole block is dead code after priority = "high" was set unconditionally on the previous line — both branches assign the same "high" value. The comment ("send our push as high priority only if...") is also stale. Consider removing the if entirely so future readers aren't confused about when high-priority is used.
Annoying! Will lave that to upstream and we'll get updates as we rebase.
Signed-off-by: Sumner Evans <[email protected]>
Still broken because the opentracing module upstream uses has long been retried and setuptools broke it.
- reduce hardcoded task parallelism - add hardcoded post task sleep
Co-authored-by: Codex <[email protected]>
5 participants
No description provided.