Repository navigation
fix(mobile-native-chat): retire an image echo glued with the send beside it - #17783
Conversation
…ide it A message sent with images could render two or three times over, with the copy carrying the photos sorting below the reply that answered it — and it never cleared. A send issued while the agent is mid-turn is glued onto the agent's input line with any send adjacent to it, so the pair lands as one transcript row whose text is the concatenation. Every retirement path then declined the pair: - The image matcher wanted the whole row to equal the echo's text, so a glued row never bound. That also stranded the local preview: the phone's photo never reached the authoritative row. - The exact-count path skips image echoes by design. - The glue path excluded image echoes too, which made one a *barrier* — splitting the run so the text-only send beside it was left alone, and a lone match is rejected as an ordinary landing. So neither echo could ever retire, and the unmatched image echo fell through to the trailing bucket, which is what put it below the reply. Match a glued row in the image matcher, and let an image echo take part in the glue pass once its preview has been rebound. It stays a barrier while unbound, so the existing guarantee is kept: an image echo never retires before its local preview reaches the transcript row, or the photo would disappear.
📝 WalkthroughWalkthroughThe change updates image preview echo matching to accept transcript rows whose normalized text starts with the preview text. It adds regression tests for concatenated matches and unrelated text. Pending retirement now treats rebound image sends as glue segments, tracks non-image exact landings separately, and retires image and adjacent text sends together. Tests also verify per-send baseline boundaries. Merge Risk: 🟡 Moderate · up to The image-echo reconciliation change can incorrectly skip a valid later matching row when an earlier image row shares the same text prefix, leaving the local preview pending. The occurrence indexing should be corrected before this PR is merged. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the bug, root cause, fix, preserved invariant, and verification results. However, it omits several required template sections, including the linked issue, visual proof or an explicit N/A explanation, the Testing section and checklist, AI Disclosure, Review, Agent skill upstream boundary, Notes, and the final Checklist. Resolution Add all required template sections. Provide the linked issue in the
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: d99ddc89-760b-4311-9216-5422d79aab2e
📒 Files selected for processing (4)
mobile/src/session/mobile-native-chat-draft-reconcile.test.tsmobile/src/session/mobile-native-chat-draft-reconcile.tsmobile/src/session/mobile-native-chat-pending-retirement.test.tsmobile/src/session/mobile-native-chat-pending-retirement.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
Greptile SummaryThe PR extends mobile native-chat reconciliation to recognize image sends glued to adjacent text sends while preserving unbound previews as retirement barriers.
Confidence Score: 4/5The PR is not yet safe to merge because image-caption prefix matching can still bind a local preview to an unrelated image-bearing transcript row. The new provenance check excludes ordinary text rows, but an unrelated image-bearing row can still satisfy the unbounded lexical-prefix predicate, receive the preview, and retire the real pending echo. Files Needing Attention: mobile/src/session/mobile-native-chat-draft-reconcile.ts
|
| Filename | Overview |
|---|---|
| mobile/src/session/mobile-native-chat-draft-reconcile.ts | Adds image-gated prefix matching, but lexical prefixes can still bind a preview to an unrelated image-bearing row. |
| mobile/src/session/mobile-native-chat-pending-retirement.ts | Allows rebound image echoes into glued retirement while retaining unbound images as barriers. |
| mobile/src/session/mobile-native-chat-draft-reconcile.test.ts | Adds glued-row and ordinary-prefix coverage, but does not exercise an unrelated image-bearing lexical-prefix row. |
| mobile/src/session/mobile-native-chat-pending-retirement.test.ts | Covers successful rebound-image glue retirement and baseline rejection. |
Reviews (2): Last reviewed commit: "fix(mobile-chat): require image provenan..." | Re-trigger Greptile
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes
Reviewed a single-commit fix that retires mobile-native-chat image echoes glued onto the agent's input line with an adjacent send. It loosens the image-echo matcher to accept prefix rows, lets a rebound image echo participate in the glue pass, and preserves the unbound-image barrier invariant.
- Image matcher accepts glued rows —
findLandedImagePreviewEchoesnow binds a caption when the row's normalized text equals it or begins with it, instead of requiring the whole row to equal the echo. - Rebound image echoes join the glue pass —
selectGluedPendingIdsgains areboundImagePendingIdsset; an image send is a barrier only while its preview is unbound, andretireLandedMobileNativeChatPendingnow passes exact landings and rebound images separately. - Tests — regression cases cover the glued-row binding, a shared-word negative, the rebound-glue path, and the preserved baseline-boundary guard.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
| // whole row to equal this echo left it unmatched, and since both other | ||
| // retirement paths skip image echoes, nothing could ever retire it. | ||
| return ( | ||
| text === targetText || (imageMessageIds.has(message.id) && text.startsWith(targetText)) |
There was a problem hiding this comment.
Lexical prefixes still misbind previews
When a pending caption such as look is followed by an unrelated image-bearing row such as looking at this [Image #1], the unbounded startsWith check selects that row, attaches the local preview to it, and retires the pending echo before its authoritative landing arrives.
| text === targetText || (imageMessageIds.has(message.id) && text.startsWith(targetText)) | |
| text === targetText || | |
| (imageMessageIds.has(message.id) && | |
| text.startsWith(targetText) && | |
| text[targetText.length] === SPACE) |
Knowledge Base Used: Mobile sessions and terminal interaction
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 18f4c9ae-00ca-4416-a293-995b6b2d53d6
📒 Files selected for processing (2)
mobile/src/session/mobile-native-chat-draft-reconcile.test.tsmobile/src/session/mobile-native-chat-draft-reconcile.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mobile/src/session/mobile-native-chat-draft-reconcile.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| return ( | ||
| text === targetText || (imageMessageIds.has(message.id) && text.startsWith(targetText)) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable repository knowledge files ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- target source ---'
cat -n mobile/src/session/mobile-native-chat-draft-reconcile.ts | sed -n '130,225p'
printf '%s\n' '--- changed hunk ---'
git diff -- mobile/src/session/mobile-native-chat-draft-reconcile.tsRepository: stablyai/orca
Length of output: 7604
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- mobile conventions ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/mobile.md
printf '%s\n' '--- mobile session learnings ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/mobile-src-session.md
printf '%s\n' '--- occurrence and reconciliation references ---'
rg -n -C 3 'expectedOccurrence|findLandedImagePreviewEchoes|baselineTailMessageId|countUserTextOccurrences' mobileRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- occurrence helper and related types ---'
cat -n mobile/src/session/mobile-native-chat-draft-reconcile.ts | sed -n '1,75p'
printf '%s\n' '--- pending occurrence construction ---'
cat -n mobile/src/session/mobile-native-chat-pending-echo.ts | sed -n '35,80p'
printf '%s\n' '--- image preview reconciliation tests ---'
cat -n mobile/src/session/mobile-native-chat-draft-reconcile.test.ts | sed -n '1,175p'Repository: stablyai/orca
Length of output: 12648
Do not count prefix matches toward expectedOccurrence.
expectedOccurrence is based on exact normalized-text counts, but the prefix branch adds image-bearing rows to the indexed candidates array. An earlier image-bearing catalog row can consume the first slot for target cat; the baseline guard then rejects it without examining the later exact row, leaving the preview pending.
Separate exact occurrence indexing from the prefix fallback.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the new commit since the prior Pullfrog review, which tightens the glued-prefix image-echo match to require image provenance.
- Provenance guard on prefix matches —
findLandedImagePreviewEchoesnow collectsimageMessageIdsfrom the raw transcript (source turns, prompt markers, or image-ref blocks) and only acceptsstartsWithwhen the candidate row carries image evidence, so a text-only row that merely begins with the caption can no longer steal the phone-local preview. - Prefix-overlap negative test — a new case proves an ordinary
look at this laterrow is not bound while the marker-bearing glued row still is.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
|
Grok Electron retry update: the exact PR worktree launched successfully with an isolated profile; renderer HTTP on port 5189 returned the Orca shell and the process identity matched this branch. CDP remained unusable: /json, /json/list, /json/version, Playwright attach, and a direct DevTools WebSocket connection all timed out with no response, so no rendered-flow screenshot or native-chat interaction could be honestly validated in Electron. The worker-owned Electron instance and validation resources were then stopped and cleaned up; no source changes were made. |
Bounded Electron CDP validation (HEAD
|
|
Validation update (current HEAD
|
…#17930) * fix(native-chat): keep the attachments on a Claude turn that pasted images (#17801) * fix(native-chat): keep the attachments on a Claude turn that pasted images A Claude turn carrying pasted images reached native chat with no images at all — no thumbnails on mobile, and not even an attachment chip on desktop. Nothing showed that the message had any. Both carriers were being dropped: - Claude records the paths in a companion turn marked `isMeta`, holding one `[Image: source: <path>]` text block per image. The decoder treats an `isMeta` user row as injected, filters it down to tool-result blocks, and returns null when none remain — so the whole row went away. - The prompt row's own `image` blocks are `{source: {type: 'base64'}}`, which carry no url or path, so `imageRefBlock` drops them too. With the companion gone, `isImageSourceUserTurn` could never fire and the fold in `normalizeImageTranscriptMessages` was unreachable on the Claude path. Surveying every transcript under `~/.claude/projects`: 238 of 241 image-source rows are `isMeta`, across every versioned release (2.1.220 through 2.1.237); the 3 that are not carry no version field at all. 38 of those rows hold more than one content block, which also defeated the single-block rule in `isImageSourceUserTurn`. Let image-source text survive the injected-turn filter, and recognize a turn whose blocks are *all* markers rather than only a lone one. An ordinary injected turn (a skill preamble, a compact summary) is still dropped, and a turn that mixes prose with a marker is still not an image-source turn. Carrying the paths keeps the payload small; decoding the base64 instead would put hundreds of KB per image on the wire to mobile. * fix(native-chat): preserve image companion ordering * fix(native-chat): keep image companions turn-local --------- Co-authored-by: Merge Sim <sim@local> * fix(native-chat): stop unjournaled provider frames from killing the session (#17813) A frame the classifier declines (status-chrome, suppressed-benign, stream-into-item) is deliberately not journaled. #17720 turned that null translation into `{accepted: false, reason: 'untranslated'}`, which is not `backpressure`, so the notification retry queue treated it as unreplayable and escalated through fail() -> forceCloseUnexpected -> connection.close(). The app-server latched `closing` and the create path's next model/list rejected with "codex app-server connection is closed (model/list)". The provider emits `remoteControl/status/changed` right after initialize, so every structured Codex session died on its first chrome frame. Admit the null translation instead, before any bookkeeping or publish. Also restore the error-frame exemption from the generic row cap, dropped by the same PR: the cap now runs after the classification check, so a noisy turn can no longer reduce provider errors to a suppression count. The test that pinned the capped behavior is inverted to assert the exemption. Co-authored-by: Merge Sim <sim@local> * refactor(mobile): split session and terminal surfaces (cherry picked from commit f129f29) * fix(mobile): restore session parity after extraction (cherry picked from commit 47706a5) * chore(mobile): prune stale max-lines overrides (cherry picked from commit be954c3663409116f34ba08658876262733b3f67) * fix(mobile): honor host-follow tab snapshots (cherry picked from commit a4a9c4da19d6967eceb8025dd01480a56261039f) * fix(mobile): keep split session hooks render-pure Documents the pre-existing render-time refs the split relocated onto changed lines, and drops a ref assignment the split added that the monolith never had. * fix(agent-send-target): acknowledge delivery even when picker closes bef (#17835) Delivery callbacks and telemetry belong to the completed send operation, not to the picker instance that launched it. Remove early returns that skipped delivery acknowledgment and success toast when the popover was already closed. * fix(mobile-native-chat): retire an image echo glued with the send beside it (#17783) * fix(mobile-native-chat): retire an image echo glued with the send beside it A message sent with images could render two or three times over, with the copy carrying the photos sorting below the reply that answered it — and it never cleared. A send issued while the agent is mid-turn is glued onto the agent's input line with any send adjacent to it, so the pair lands as one transcript row whose text is the concatenation. Every retirement path then declined the pair: - The image matcher wanted the whole row to equal the echo's text, so a glued row never bound. That also stranded the local preview: the phone's photo never reached the authoritative row. - The exact-count path skips image echoes by design. - The glue path excluded image echoes too, which made one a *barrier* — splitting the run so the text-only send beside it was left alone, and a lone match is rejected as an ordinary landing. So neither echo could ever retire, and the unmatched image echo fell through to the trailing bucket, which is what put it below the reply. Match a glued row in the image matcher, and let an image echo take part in the glue pass once its preview has been rebound. It stays a barrier while unbound, so the existing guarantee is kept: an image echo never retires before its local preview reaches the transcript row, or the photo would disappear. * fix(mobile-chat): require image provenance for glued prefix matches --------- Co-authored-by: Merge Sim <sim@local> * refactor(renderer): split oversized UI surfaces (cherry picked from commit da89be4) * fix(renderer): keep split imports lint-clean (cherry picked from commit c778ac7) * fix(renderer): preserve extracted lifecycle and retention behavior (cherry picked from commit 1731f2a) * chore(max-lines): prune refactored file suppressions (cherry picked from commit 787228e) * fix(status-bar): preserve Git refresh ordering after split (cherry picked from commit e65d298) * fix(lint): preserve TaskPage effect suppressions after split (cherry picked from commit 4a3bc23) * fix(status-bar): preserve workspace-space review and agent freshness (cherry picked from commit 9455e31) * fix(automations): preserve Escape drill-out behavior (cherry picked from commit 790680d) * fix(renderer): restore worktree palette behavior after split (cherry picked from commit 0b78c1c) * style(renderer): format palette project candidates (cherry picked from commit 1e93d66) * refactor(renderer): finish worktree palette extraction (cherry picked from commit ea8e6e5) * fix(renderer): document intentional render-time refs (cherry picked from commit 6fc78dd) * chore(max-lines): prune stale lifecycle baseline (cherry picked from commit 42bf3eb) * fix(renderer): preserve status localization parity (cherry picked from commit 0a802c7) * fix(renderer): preserve floating editor visibility (cherry picked from commit bcf715b) * fix(automations): preserve destination form save semantics (cherry picked from commit a793b07) * fix(renderer): preserve Codex sign-in behavior (cherry picked from commit 81f4803) * fix(renderer): preserve palette create telemetry (cherry picked from commit b13c2b1) * style(renderer): format extracted palette modules * refactor(automations): extract list toolbar and drop dead list projection * fix(lint): drop unused exhaustive-deps suppression after split The split's dependency list no longer trips the rule, so the directive is dead and the changed-code quality gate rejects it. * fix(renderer): document pre-split render-time refs in workspace search Both assignments are verbatim from the pre-split monolith; the split relocated them onto changed lines, which the React Doctor gate scans. * refactor(renderer): split runtime and store modules (cherry picked from commit 207bef0) * fix(renderer): merge split runtime imports (cherry picked from commit 90bf505) * chore(max-lines): prune runtime baseline entries (cherry picked from commit 08820a4) * fix(renderer): preserve accepted visibility snapshots (cherry picked from commit bc5f8a5ac5b8e8ad3fc59250795842e663bad0ee) * refactor(preload): split bridge API modules (cherry picked from commit b77e318) * test(preload): census split GitHub bridge owners (cherry picked from commit cd08e4e) * fix(preload): merge split plugin imports (cherry picked from commit 682ff4f) * chore(max-lines): prune preload baseline entry (cherry picked from commit aad2e1e) * fix(pty,remote): close the pty master fd leak and two remote-terminal defects #8362: node-pty leaves the forkpty()/posix_openpt() master without FD_CLOEXEC, so on Linux every later child of the process -- both later pty children and plain child_process spawns -- inherits it and keeps the /dev/pts device alive. Measured on Linux with stock node-pty 1.1.0: master fd flags 0404002 (cloexec=false), and 17 -> /dev/pts/ptmx present in both a later pty child's /proc/self/fd and a later child_process child's. Extend the existing node-pty patch with pty_cloexec() on both PtyFork spawn paths; after the patch the flags read 02404002 (cloexec=true) and neither child sees the master. This covers the app and terminal daemon only -- the SSH relay installs node-pty from npm on the remote host, so it stays exposed (see the report). #8414: the inspection queue's fire-and-forget `.finally()` chain re-raised a rejecting inspection as a renderer-global unhandledrejection, which an unreachable runtime produced on every cadence tick. #9194: `tab_not_found` is the host's definitive "no such tab", but the close path cleared the close intent for it exactly like a dropped connection, so a host that keeps republishing the dead surface re-materialized the pane the user just closed. Keep that intent and drop its TTL. Also route the banner's "Remote terminal was closed." line through translate() so it stops mixing English into a localized banner. * fix(pty): retire the node-pty master fd instead of probing around it node-pty hands the master to libuv, which closes it on EIO/EOF, but `_close()` never invalidated `_fd` and neither `resize()` nor the `process` getter consulted anything. A late ioctl therefore addressed whatever the kernel had since reissued that number to. On Linux the very next pty gets it: resizing an exited handle silently resized an unrelated live pane, with no error for #17832's probe-retry-classify to observe. Retire `_fd` in the same block that gives up the master and guard both fd-addressed syscalls on it, so a stale handle is unreachable by construction. The relay's resize keeps the isProcessAlive-proven retirement — still the only evidence of `exited` — and drops the re-probe-and-classify arm the ioctl no longer needs. Windows is unaffected: WindowsTerminal.resize goes through the conpty agent and reads no fd. * fix(pty): retire the write stream's fd copy too, and keep the resize re-probe Two gaps in the fd-retirement change. `CustomWriteStream` is the third fd-addressed surface and takes its own plain-number copy of the master at spawn, so `Terminal._fd = -1` never reaches it. Its `dispose()` only cleared a pending immediate, and it was called from `destroy()` alone — the EIO/EOF read-error path and the socket `'close'` path both reach `_close()` without it. A residual `_writeQueue` and an in-flight `fs.write` therefore kept addressing the number after libuv gave it up. Retire the copy and drop the queue in `dispose()`, and call it from `_close()`. Restore the relay's catch-block re-probe. It was never redundant with the patch: libuv closes the fd synchronously inside `uv_close`, before the JS `'close'` that runs `_close()`, so the reuse window opens ahead of any JS guard; and a relay host installs node-pty from npm, where the patch is not applied at all. The re-probe is a `process.kill(pid, 0)` on a path already inside an exception handler. Both existing resize tests pinned liveness with a constant mock, which is why the deletion passed CI invisibly; the new case is alive at the pre-probe and gone by the time the ioctl throws. Also adds the patch's first upstream reference: microsoft/node-pty#220 named this mechanism in 2018 and was closed 2025-12-19 as completed after only improving the error message; #827 is still open. Tracked as #18109. * test(pty): release the live pty before the next fd-reuse case The new Linux write case killed its live pty without awaiting the exit, so that master was still open when the next case spawned. Linux hands out the lowest free descriptor, so the retired pty got N+1 while the leaked N closed during the 400ms settle -- and `does not name a live pty foreground process off the retired descriptor` then saw its live pty take N, failing its own reuse premise by exactly one (`expected 22 to be 23` on CI, reproduced as 19/20 in a container). Test isolation, not a Linux behavior difference: with the descriptor released the case passes 8/8. Await the exit like the resize case above already does, and switch the payload command to `sleep 1` -- the line discipline echoes a leaked write back on its own, so nothing has to read it. Note the constraint on the block so the next case does not repeat it. The premise assertion stays as it is. It compares two observed descriptors, not a hardcoded number; relaxing it to "differs from the retired one" would let both reuse cases pass vacuously on a kernel that never reissues, which is the whole thing they exist to demonstrate. Verified with vitest under node:24-bookworm against a source-built patched node-pty: 7 passed, 8 runs, no flake. Negative check in the same container -- reverting the `_close()` -> `dispose()` hook fails the case with `expected 'leak\r\n' not to contain 'leak'`, so the rewritten assertion is still load-bearing. --------- Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Co-authored-by: Merge Sim <sim@local> Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
…ide it (stablyai#17783) * fix(mobile-native-chat): retire an image echo glued with the send beside it A message sent with images could render two or three times over, with the copy carrying the photos sorting below the reply that answered it — and it never cleared. A send issued while the agent is mid-turn is glued onto the agent's input line with any send adjacent to it, so the pair lands as one transcript row whose text is the concatenation. Every retirement path then declined the pair: - The image matcher wanted the whole row to equal the echo's text, so a glued row never bound. That also stranded the local preview: the phone's photo never reached the authoritative row. - The exact-count path skips image echoes by design. - The glue path excluded image echoes too, which made one a *barrier* — splitting the run so the text-only send beside it was left alone, and a lone match is rejected as an ordinary landing. So neither echo could ever retire, and the unmatched image echo fell through to the trailing bucket, which is what put it below the reply. Match a glued row in the image matcher, and let an image echo take part in the glue pass once its preview has been rebound. It stays a barrier while unbound, so the existing guarantee is kept: an image echo never retires before its local preview reaches the transcript row, or the photo would disappear. * fix(mobile-chat): require image provenance for glued prefix matches --------- Co-authored-by: Merge Sim <sim@local>



What was broken
A message sent with images rendered two or three times over in mobile native chat, and the copy carrying the photos sorted below the assistant reply that answered it. It never cleared.
Root cause
A send issued while the agent is mid-turn is glued onto the agent's input line with any send adjacent to it, so the pair lands as one transcript row whose text is the concatenation. In the reported case the row's text was
A \r [Image #1..4] B— one send carrying the caption + photos, one carrying the follow-up question.Every retirement path then declined the pair:
mobile-native-chat-draft-reconcile.ts:168)mobile-native-chat-pending-retirement.ts:157):49)matchGluedRunrejects a lone match (matched > 1at:129)So neither echo could ever retire. The unmatched image echo then had no usable anchor and fell through to
trailingPending, which is what put it below the reply.Confirmed empirically before fixing — feeding the real shape through
findLandedImagePreviewEchoes+retireLandedMobileNativeChatPendinggaveLANDED: []andSURVIVORS: ["p1","p2"].The fix
startsWithwith an optional single space), andclaimedMessageIdsplus the baseline-tail guard still prevent over-claiming.The guarantee that is preserved
An image echo stays a barrier while its preview is unbound. The existing invariant —
keeps a captioned image echo and its neighbors until the preview is rebound— matters: retiring the echo before the local preview URI is bound to the transcript row would drop the phone-local photo, which the row's host path cannot render.My first attempt let image sends glue unconditionally and broke exactly that test. The landed version gates glue participation on
reboundImagePendingIds, so the invariant holds and the suite is green.Verification
tsc --noEmit: clean.Review note
This module has history — #14665 landed glued retirement, #14819 reverted it, #14936 relanded it. I kept the change additive to the existing rules rather than restructuring them, and pinned the invariant that the revert was about.