Repository navigation
Fix four defects in the model-badge integration - #167
Merged
Merged
Conversation
1. HIGH: a display-name relogin left the caller on the rejected token. mx.client::mx_set_displayname() wraps mx_with_relogin(), so a stale token is refreshed and persisted, but it returns invisible(TRUE) and discards the refreshed client. matrix_update_displayname() kept its own cfg, and the /model acknowledgement then sent with the token the homeserver had just rejected. chat_send() does no relogin of its own, so the failure was swallowed by the caller's tryCatch and the ack vanished with nothing logged. It now returns a config and re-reads after a successful rename; all three call sites adopt it. 2. Startup and /clear advertised the wrong model under runtime overrides. Both call matrix_update_displayname() with no session, so matrix_badge_displayname() fell back to cfg$model rather than the model the next session would actually be built with. Both now pass the run's model/provider. 3. The mx.client >= 0.2.0 floor was declared in DESCRIPTION but not enforced at runtime. matrix_require_mx() version-gated chat.api only, with a comment explaining exactly why a Suggests floor is not a runtime guarantee -- and did not apply it to mx.client. A host on 0.1.1 passed the guard and then lost the rename to a best-effort tryCatch. Now gated on .MX_CLIENT_MIN. 4. The badge tests only covered pure rendering helpers, so 1 and 2 both escaped them. Added guards that drive matrix_update_displayname() with a stubbed relogin, assert the override reaches the display name, and check the runtime floor.
1. HIGH: the refreshed token never reached encrypted sends. matrix_send_maybe_encrypted() adopted the fresh cfg for the member lookup but still handed mx_send_encrypted() crypto$client, a copy taken once at init. After a rename relogin that object holds the rejected token, so an encrypted /model acknowledgement was dropped exactly as before. The cached client is gone; sends take the caller's live cfg. Nothing else read the field. 2. Startup returned a stale mx_sess: it was built before the rename and returned after, so archive-flush room-name lookups spent the run on a rejected token. Rebuilt after the refresh. 3. The regression tests did not reach the regression. They called matrix_update_displayname() directly, so deleting any of the three 'cfg <-' assignments left them green. There is now a matrix_poll() test that drives a /model switch through a relogin-on-rename and asserts the send receives the rotated token; reverting the assignment turns it red, which I verified rather than assumed. Writing it surfaced why the first attempt was so easy to get wrong: matrix_poll() ignores chat_poll()$messages and re-extracts from res$raw, so a message seeded through the .extract seam never reaches the reply path at all. 4. The floor test only proved CI had a new enough mx.client. The version lookups in matrix_require_mx() are now injectable, so the gate is tested by passing an old version and expecting the error.
Three review rounds have each found the same defect in a different holder, so this fixes the class rather than the instances. The token rotates -- chat_poll() relogins, and so does a badge rename -- and anything built once from cfg keeps the rejected one. matrix_mx_session() is pure field validation over cfg, no network, so caching it buys nothing and costs correctness. The poll loop now derives through sess_now()/chat_now() at each use: read receipts, member lookups, archive, typing. matrix_run_step() derives its own session for the archive flush rather than using the startup one, which every rotation since had left stale. matrix_approval_cb() closed over the cfg its session was built with, and a session outlives many rotations. A prompt sent on a rejected token fails into FALSE, which the model reads as the user declining. It now re-reads at invocation; approvals are rare, interactive and already blocking, so the read is free. Swept the rest: every remaining mx_sess is either call-scoped or the deliberate pre-sync chat client, which must exist before the sync. Tests: the E2EE guard drives matrix_send_maybe_encrypted() and asserts mx_send_encrypted() gets the live cfg -- reintroducing crypto$client turns it red, which I checked. /clear now has the same relogin guard as /model. Updated the typing source-pin, whose intent (typing rides the contract, not mx_typing) is unchanged.
mx_with_relogin() persists the refreshed client BEFORE it retries, so a relogin whose retry then fails -- rate limit, transient 5xx -- still leaves the live token on disk. Gating the reload on success threw it away and sent the following /model or /clear acknowledgement on the rejected token: the same silent drop the reload was added to prevent. Disk is authoritative either way, so the success check went away rather than growing another branch. When nothing rotated the reload returns what we already had. The earlier guards only modelled 'persist, then succeed', which is why this survived two rounds. The new one models 'persist, then throw'; restoring the success gate turns it red, verified. Also drops the now-unused mx_sess assignment left by the de-caching.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review findings against the #155 rescue. The chat.api transport rewire itself was clean; all four are in the badge work.
1. HIGH — a display-name relogin left the caller on the rejected token
mx.client::mx_set_displayname()wrapsmx_with_relogin(), so a stale token is refreshed and persisted — but it returnsinvisible(TRUE)and discards the refreshed client.matrix_update_displayname()kept its owncfg, so the/modelacknowledgement then sent with the token the homeserver had just rejected.chat_send()does no relogin of its own, so the failure was swallowed by the caller'stryCatchand the acknowledgement vanished with nothing logged. Narrow window, but token rotation is the entire reason this path exists.Now returns a config and re-reads after a successful rename; all three call sites adopt it.
Follow-up for mx.client: the real fix is for
mx_set_displayname()to return the client the waymx_with_relogin()does. That is an mx.client change and 0.2.0 is mid-submission, so corteza reloads from disk for now.2. Startup and /clear advertised the wrong model under runtime overrides
Both call
matrix_update_displayname()with no session, somatrix_badge_displayname()fell back tocfg$modelrather than the model the next session is actually built with:Per-message badges were always right; only the account display name was wrong. Both sites now pass the run's
model/provider.3. The mx.client floor was declared but not enforced
matrix_require_mx()version-gated chat.api only — with a comment explaining precisely why a Suggests floor is a resolution hint and not a runtime guarantee, and then not applying that to mx.client. A host on 0.1.1 passed the guard, and the missingmx_set_displayname()was lost to a best-efforttryCatch, so the sender-line badge silently never worked. Now gated on.MX_CLIENT_MIN.4. The tests did not reach these paths
The 15 badge assertions covered pure rendering and default helpers. None drove
matrix_update_displayname()or/modeland/clearthrough the rewired transport, which is exactly why 1 and 2 escaped. I had claimed "2663 tests pass" settled the semantic merge; it settled the helpers, not the integration.Added guards that drive
matrix_update_displayname()with a stubbed relogin and assert the reloaded config is adopted, assert the runtime override reaches the display name, and check the runtime floor.Verification
2668 tests, 0 failures.
R CMD check: 0 errors, 0 warnings, 1 NOTE (Suggests or Enhances not in mainstream repositories: chat.api, the known drat item).