Skip to content

fix(native-chat): anchor literal image-marker stripping to a real image send (STA-4363) - #14939

Closed
brennanb2025 wants to merge 45 commits into
mainfrom
brennanb2025/sta-4363-literal-image-markers-focused
Closed

brennanb2025 wants to merge 45 commits into
mainfrom
brennanb2025/sta-4363-literal-image-markers-focused

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Aug 16, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 24 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1990 $\color{#cf222e}{\Huge{\mathbf{−}}}$​28 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1962
Prod 27 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​758 $\color{#cf222e}{\Huge{\mathbf{−}}}$​150 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​608

ELI5

If you type [Image #1] as ordinary text, native chat used to delete it from what
you see. It only looks deleted — the stored transcript is fine — but your own
words disappearing from your own message is jarring. Markers are now stripped only
from the prompt that a real image upload is actually attached to.

What Changed

normalizeImageTranscriptMessages stripped [Image #N] from every user turn,
including turns with no image behind them. Stripping is now anchored: it happens
only on the prompt that immediately follows a contiguous, same-source
[Image: source: …] run. A standalone turn keeps the user's literal text.

Once rendering became provenance-aware, the optimistic-echo keys had to move with
it. A marker-only turn previously normalized to the empty string and produced a
null key, so its queued bubble could never match the landed turn and the row
duplicated. Desktop and mobile now share one rule (nativeChatUserMessageMatchText):
a marker run on a turn carrying no image is the user's own text and keys
verbatim; a turn carrying images keys with markers stripped, as before.

Anchoring alone was not enough. It decided which turn gets stripped but never
how many markers may go — the strip was a global regex. So a turn that attaches
one image and also mentions a second marker in prose still lost the user's words:

[Image #1] compare with the [Image #2] I mentioned earlier

[Image #2] is the user's own text — there is no second image — and it was
deleted. That is this ticket's own defect, surviving inside the fix for it. The
strip is now bounded by the size of the contiguous image-source run it anchors to:
at most one marker per image, taken in document order across the turn's text
blocks, with the remainder left verbatim. Position rather than ordinal decides
which markers are placeholders, so the rule holds whether the producing CLI
numbers markers per-turn or per-session. When the structural evidence is shorter
than the marker count the failure is now closed — surplus markers render as the
plain text they are — instead of open.

One more layer could still eat the text. Preserved markers render through
CommonMark, where a line of the form [Image #1]: /tmp/a.png is a link reference
definition
and produces no output at all; a user turn consisting only of that line
rendered as an empty bubble. Measured against the real renderer, not assumed.

The first attempt escaped the line-leading bracket in the source text. It was
replaced, because escaping only reaches lines at the top level — a definition
inside a list item or a blockquote, or one whose label wraps across lines, is
swallowed anyway. User turns now render with the definition and
gfmFootnoteDefinition constructs switched off at the parser
(remarkDisableDefinitions, opted into per call site through
disableLinkDefinitions). That covers every container without rewriting a byte of
what the user wrote. Assistant prose is untouched and keeps its definitions, and
inline links and the rest of GFM are unaffected either way.

Relationship to #14581

This supersedes #14581, which fixes the same ticket. Do not merge both.

Kept (cherry-picked from #14581, with -x provenance on each commit): the
anchoring fix itself and the echo-key coherence work that must accompany it —
fab320e42, 7803de9c1b, 94f1cc66ee, c51e8e6bb, 794e07c8d, 9b9aa6f97,
d49d6ad02, bf74570db. That work is sound and its tests are outcome-based.

Discarded, with reasons:

  • The Claude image-source metadata feature (b5ba22e46, e9b692fe8,
    f74297c45, bc1bebadd, 604748f58). These retain [Image: source: …]
    sidecars the host previously dropped, decode multi-image metadata, and then
    necessarily add a NATIVE_CHAT_IMAGE_SOURCE_RUNTIME_CAPABILITY negotiation plus
    a legacy wire projection to keep old clients correct. The negotiation is
    well-built, but it is a Rule 3 wire change under
    docs/reference/remote-wire-compatibility.md and it is a separate feature from
    STA-4363 — 676 of fix(native-chat): preserve literal image markers #14581's production lines against 322 for the fix. It deserves
    its own PR and its own cross-version review, not a ride-along in a P2 display fix.
  • 48f175aa7a "fold prompt-first image transcripts", specifically. It lets a
    marker-bearing turn absorb an image-source run that follows it, matched only by
    marker count. That reintroduces this very ticket's defect through a new door: a
    literal [Image #1] the user typed, immediately followed by a genuinely separate
    image send, loses its marker and absorbs the wrong image. Covered now by
    "does not let a literal marker turn claim a later send's image source".
  • 824649228d "fix(ci): satisfy PR LoC type-aware audit". It edits
    config/scripts/pr-test-loc-summary.test.mjs, a file introduced by ci: post test vs non-test LoC on pull requests #14738; it has
    no relationship to native chat, and main has since landed the byte-identical
    change independently.

Also note: #14581 no longer merges into main. It builds on #14665, which main
reverted in #14819, producing two modify/delete conflicts that would resurrect
reverted code. This branch is cut from current main and re-homes the one helper
(userTextOccurrenceCounts) that lived in the reverted module.

Known remaining path

A turn with markers but no image ref still counts as evidence of an image — the
conflation this PR otherwise ends — so its content key (text:<stripped> +
images:1) is byte-identical to a genuine one-image send's key. Two collisions in
that area were found and fixed on this branch rather than deferred.

One row minting two keys. A row like [Image #1] fix produced both readings —
the user's own literal text, and a host echoing an image send as that marker — and
both count builders credited each, so one transcript row retired two pending sends.
A row showing no photo could retire a send that carried one, and prunePendingSends
then dropped it from the bounded cache with the user's photo in it.
literalContentKeysClaimedByPendings resolves this at mint time: a send that
carried no image owns the literal reading, so when one is waiting on that reading it
wins and the row stops there. Otherwise the marker reading stands, which is what
keeps a marker-only host echo retiring its send.

Markers outnumbering the photos on the same row. The evidence count was
max(imageRefCount, markerCount). That was equivalent while every marker was
stripped from every turn — but this branch bounds the strip to the size of the run
it anchors to, so a row can legitimately keep markers the user typed, and the max
then read them as extra photos. A one-image run whose prompt is
[Image #1] look [Image #2] [Image #3] folds to one image-ref plus two surviving
markers and reported evidence for two photos while rendering one, which is the key a
two-photo send owns. Structure now wins wherever it exists; markers remain the
fallback only for a row carrying no image ref at all, which is the marker-only host
echo they were added for. Under-counting costs a duplicate bubble; over-counting
loses the photo — the same trade the glue arm already makes.

Correcting the earlier record on what the marker fallback protects. This section
previously said it protected the paginated-window case: the read window showing a
prompt turn while its [Image: source: …] turn is still unloaded. That is wrong on
every current transport. Pruning runs against post-fold messages, so wherever the
host reports its paging answer the fold has already stripped the window head's
markers and no marker-derived key is minted there at all. What the fallback actually
protects is the marker-only host echo — a host echoing an image send as a bare
[Image #n] with no source turn anywhere in the window. The original description
survives only against a host predating the reported paging field, where the head
rule stays off entirely.

Giving the key path the same adjacency provenance the render path uses remains a
follow-up: it changes the function's signature across both surfaces and several call
sites, and with both guards in place the remaining exposure is a duplicate bubble
rather than data loss.

Why

Transcript structure is the only authoritative proof that an [Image #N] belongs
to an image. Local preview state must not decide what the user's text says, and a
render path and a reconciliation path that disagree about what a marker means
cannot reconcile at all.

Linked Issue

STA-4363 — native chat strips literal
[Image #N] from user messages. Regression from #14162.

Testing

  • pnpm vitest run --config config/vitest.config.ts src/renderer/src/components/native-chat src/shared/native-chat — 92 files / 1010 tests passed.
  • cd mobile && npx vitest run --config vitest.config.ts src/session/ — 134 files / 1240 tests passed.
    (Use this form, not pnpm --dir mobile exec …: that rewrites mobile/pnpm-lock.yaml
    and drops patchedDependencies.)
  • Every changed file, including the four outside the two paths above (main IPC, the
    sidebar Markdown plugin, the web preload provider surface, the command envelope) —
    5 files / 41 tests passed.
  • Every test file that mentions native chat by CONTENT rather than path — 153 files /
    2841 tests passed. Path filters miss these: three CI-reds on this branch hid behind
    a native-chat filter because the file paths contain neither native-chat nor
    nativeChat.
  • pnpm typecheck and mobile tsc --noEmit — both exit 0, with node_modules
    confirmed present (the typecheck script exits 0 without it).
  • Root and mobile oxlint clean; gate verified live by injecting a violation and
    confirming it was reported. oxfmt applied. No max-lines suppression added.
  • Failing-first. The count-bound tests were written before the fix and run
    against the branch head: 4 of the 5 new marker tests failed there (the fifth
    pins count-over-ordinal semantics, which the old code already satisfied). The
    Markdown swallow was likewise measured against the live renderer before the fix
    existed — [Image #1]: /tmp/a.png rendered to an empty <div>.
  • Non-vacuity, measured. Each load-bearing line was reverted one at a time and
    the suites re-run; every applicable mutation went RED, and none failed on a
    missing export — every failure was a behavioural assertion. Across the branch
    that is 26 RED. For the two most recent fixes specifically:
    • the evidence-count fix: reverting only that line turns exactly its 2 new tests
      RED while 123 others in the same files stay green. Confirmed a second way, by a
      reviewer who restored the old implementation through a module mock rather than
      editing the source, and a third way by scoring the new tests against four
      mutant implementations to establish which each one kills.
    • the mobile send-origin wiring: dropping the images argument turns exactly
      ONE test RED out of 1240 in the mobile suite — the one added for it.
      Mobile parity is real, not incidental: mobile re-exports the shared module
      through a barrel and its suite goes RED on the shared mutations.
  • Base-vs-head differentials. 163,856 cases through the real prune pipeline,
    each side run through its OWN fold: cases where this branch loses a photo the
    base kept fall from 408 to 288 after the evidence-count fix, with none in the
    other direction and no case of one transcript row retiring two sends. An
    independent 40,000-case fuzz moves head-only photo loss from 2 to 1, with zero
    cases where the fix loses a photo the previous build kept.
  • Not manually exercised on Linux, Windows, or SSH. This is transcript
    normalization and renderer-local echo bookkeeping: no platform APIs, paths,
    shortcuts, or wire changes, and no local-worktree or folder-workspace assumptions.

AI Disclosure

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered
  • Focused lint, typecheck, and tests pass locally

Author

  • X / Twitter: @BrennanKB5

The render path became provenance-aware for `[Image #n]` markers, but the
pending-key path kept stripping them unconditionally, so the two disagreed:
a standalone marker-only turn rendered its literal text while its content key
computed to empty, leaving the optimistic echo unmatchable and the row
duplicated on desktop and mobile.

Give both sides the same rule via `nativeChatUserMessageMatchText` — a marker
run that stripping would erase, on a turn carrying no image, is the user's own
text and keys verbatim. Mobile's preview reconciliation also accepted a single
marker as proof for any number of pending images, so an incomplete multi-image
echo could clear or rebind every preview; it now requires one marker per image.

(cherry picked from commit 7803de9)
…e run

Re-home the landed literal-text counts onto the draft reconciler after the
#14665 revert removed the module they lived in, and cover the anchoring
boundary: a marker the user typed must not absorb a later send's image.
@coderabbitai

coderabbitai Bot commented Aug 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ed59eff4-b28a-41a8-9024-d91a8242b19d

📥 Commits

Reviewing files that changed from the base of the PR and between 2719292 and 0f93e95.

📒 Files selected for processing (4)
  • mobile/src/session/mobile-native-chat-render-data.test.ts
  • mobile/src/session/mobile-native-chat-render-data.ts
  • src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts
  • src/renderer/src/components/native-chat/native-chat-pending.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • mobile/src/session/mobile-native-chat-render-data.ts
  • mobile/src/session/mobile-native-chat-render-data.test.ts
  • src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts
  • src/renderer/src/components/native-chat/native-chat-pending.test.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Native chat now distinguishes literal [Image #n] text from image-backed transcript markers. Shared utilities count image markers and image evidence, preserve surplus markers, and support truncated history. Mobile drafts and renderer pending matching use image counts for reconciliation and preview retention. Message assembly propagates history state. User rendering preserves marker and definition-shaped text. Markdown rendering can disable link-definition parsing. Tests cover these behaviors.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is detailed, but it omits the required Visual Proof section and does not provide before-and-after evidence for this behavior change. Add the required Visual Proof section with before-and-after screenshots or videos, and complete any other omitted template sections.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main fix: preventing literal image markers from being stripped unless tied to a real image send.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch brennanb2025/sta-4363-literal-image-markers-focused

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Anchoring decided which turn gets stripped but not how many markers may go,
so a turn attaching one image and mentioning a second marker in prose still
lost the user's literal text. Strip at most one marker per image in the
anchored source run, in document order across the turn's text blocks; markers
past the count stay verbatim.

Position, not ordinal, decides which markers are placeholders, so the rule
holds whether the producing CLI numbers markers per-turn or per-session.
… mouth

A preserved literal marker can still vanish one layer later: CommonMark reads
`[Image #1]: /tmp/a.png` as a link reference definition, so a user turn that is
only that line renders as an empty bubble. Escape the line-leading bracket on
user turns before they reach the Markdown renderer.

Assistant prose is untouched, and the escape is output-neutral everywhere else,
including inside fenced code.
Escaping a line-leading bracket only reached definitions at the top level.
Measured against the live renderer, a definition-shaped marker line still
rendered as an empty bubble inside a bullet, an ordered item or a blockquote,
and whenever the label wrapped across lines — and the escape injected a visible
backslash into a fenced block whose fence characters differed from an inner
fence line.

Turn the construct off for user rows instead, so every container is covered and
the source text is never rewritten. Inline links, reference links, GFM and
fenced code all render byte-identically.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
src/shared/native-chat-image-transcript-markers.ts (1)

150-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse countMarkers instead of repeating the match expression.

countImagePromptMarkers repeats the match(IMAGE_PROMPT_MARKERS)?.length ?? 0 expression already defined as countMarkers at Line 30. One helper keeps the two counts in step if the regex or the counting rule changes.

♻️ Proposed refactor
 export function countImagePromptMarkers(message: NativeChatMessage): number {
   return message.blocks.reduce(
-    (count, block) =>
-      count + (isTextBlock(block) ? (block.text.match(IMAGE_PROMPT_MARKERS)?.length ?? 0) : 0),
+    (count, block) => count + (isTextBlock(block) ? countMarkers(block.text) : 0),
     0
   )
 }
src/shared/native-chat-image-transcript-markers.test.ts (1)

262-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider adding a case for mid-text stripping whitespace.

The suite covers leading, trailing, and surplus markers. It does not cover a marker between two words, where stripImagePromptMarkersUpTo removes the marker but leaves both surrounding spaces. A case such as 'look [Image #1] here' pins the exact rendered spacing and prevents a silent change later.

src/renderer/src/components/native-chat/native-chat-user-turn-render.test.tsx (1)

45-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Interpolate the case into the it.each title.

All four cases share one title, so a failure report does not say which input failed. Vitest substitutes %s with the case value.

♻️ Proposed refactor
-  ])('shows a definition-shaped user line on screen', (typed) => {
+  ])('shows a definition-shaped user line on screen: %j', (typed) => {

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f72447e7-4a8e-48ca-bfa4-1d439a56e861

📥 Commits

Reviewing files that changed from the base of the PR and between bf6dc6f and 4fa5a62.

📒 Files selected for processing (22)
  • mobile/src/session/mobile-native-chat-draft-reconcile.test.ts
  • mobile/src/session/mobile-native-chat-draft-reconcile.ts
  • mobile/src/session/mobile-native-chat-image-transcript-markers.ts
  • mobile/src/session/mobile-native-chat-literal-marker-echo.test.ts
  • mobile/src/session/mobile-native-chat-pending-echo.ts
  • mobile/src/session/mobile-native-chat-render-data.test.ts
  • mobile/src/session/use-mobile-native-chat-drafts-launch-draft.test.ts
  • mobile/src/session/use-mobile-native-chat-drafts.test.ts
  • mobile/src/session/use-mobile-native-chat-drafts.ts
  • mobile/src/session/use-mobile-native-chat-launch-draft-seed.ts
  • mobile/src/session/use-mobile-native-chat-message-send.ts
  • src/renderer/src/components/native-chat/NativeChatMessageList.tsx
  • src/renderer/src/components/native-chat/native-chat-literal-marker-echo.test.ts
  • src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts
  • src/renderer/src/components/native-chat/native-chat-pending.test.ts
  • src/renderer/src/components/native-chat/native-chat-session-assembler.test.ts
  • src/renderer/src/components/native-chat/native-chat-user-turn-render.test.tsx
  • src/renderer/src/components/sidebar/CommentMarkdown.tsx
  • src/renderer/src/components/sidebar/remark-disable-definitions.test.tsx
  • src/renderer/src/components/sidebar/remark-disable-definitions.ts
  • src/shared/native-chat-image-transcript-markers.test.ts
  • src/shared/native-chat-image-transcript-markers.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 1 remains after this review.

Comment thread src/shared/native-chat-image-transcript-markers.ts
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review pass — visual proof + one fix pushed

Ran on an isolated dev instance (own profile, own CDP + renderer ports, identity re-verified as
devRepoRoot = …/loop2-14939 before every capture), driving the real Claude Code agent through the real
native chat composer. No computer-use, no OS-level input automation.

A literal [Image #1] with no attachment survives

literal marker survives

The user turn renders Reply with just OK. Do not use tools. [Image #1] — the marker the user typed is
still there. That is the ticket.

A turn that is only [Image #1]: /tmp/a.png renders

definition-shaped line renders

Both turns at once: the literal-marker turn and the definition-shaped turn. Before this PR the second one
rendered as an empty bubble — CommonMark reads the line as a link reference definition and emits
nothing.

Follow-up pushed: the escape did not reach block containers

bullet list definition line renders

The bullet [Image #1]: /tmp/a.png above renders as typed. It did not before commit 4fa5a62.

I measured the branch's original regex pre-pass against the live renderer. Rendered text, empty string =
the user's line is gone from the screen:

user typed before this PR with the regex escape now
[Image #1]: /tmp/a.png (empty) renders renders
- [Image #1]: /tmp/a.png (empty) (still empty) renders
1. [Image #1]: /tmp/a.png (empty) (still empty) renders
> [Image #1]: /tmp/a.png (empty) (still empty) renders
[Image #1⏎still mine]: /tmp/a.png (empty) (still empty) renders
- [Image #1]: /tmp/a.png⏎- second second second both items

The escape also introduced a defect: for a ~~~-fenced block containing a ``` line, it emitted a
visible \[ into the code block, because the fence tracker flips on any fence marker regardless of type.

Root cause of the gap: escaping a line-leading [ can only see column 0 of the top level. A link reference
definition is a block construct — it is equally valid inside a list item or a blockquote, and its label
may wrap across lines. So 4fa5a62 stops parsing the construct at all for user rows
(disableLinkDefinitions, opt-in on CommentMarkdown, set only by the native-chat user row) and deletes
the string-rewriting pass. Inline links, reference links, GFM tables and fenced code all render
byte-identically; the single intended difference is that an authored [d]: url line in a user turn now
shows as text instead of vanishing, which is the desired behaviour for prose.

Non-vacuity

Reverting production only and keeping every test: 29 tests go RED on behaviour (17 desktop, 12 mobile).
Reported separately, and not counted above: 13 further desktop failures that are missing-export/compile
errors rather than behavioural evidence (12 from a test file whose module was deleted, 1 … is not a function). One desktop timeout in src/main/ipc/native-chat.test.ts was load-induced — that file is not
touched by this PR and passes at HEAD.

The two new test files were mutation-checked: removing disableLinkDefinitions from the user row turns
4 of 5 wiring tests RED.

The deferred nativeChatUserMessageImageEvidenceCount item — re-derived, keep it

Making the count image-derived turns 4 existing tests RED, not the 2 recorded earlier:

  • desktop drops an attachment pending send once a prefixed transcript prompt advances (+ trailing-marker twin)
  • mobile reconciles an image-only unconfirmed send against the next user turn (no false warning)
  • mobile hands a captioned preview to a marker-only compatibility echo

They encode the paginated-window case: the read window can show the prompt turn while its
[Image: source: …] turn is still unloaded, so markers are the only evidence a real image send landed.
Dropping the marker term trades an unreachable collision for a permanent duplicate bubble. Keep it.

Not proven

  • No BEFORE screenshot. The dev instance was SIGTERM'd by something outside this session twice mid-run, and
    swapping the render path under a live dev server left the workbench's lazy chunk unloadable. The BEFORE
    column in the table above is measured against the live renderer, not screenshotted.
  • A genuine image send (markers still stripped, no over-correction) and the one-image-plus-second-marker
    case are covered by tests, not by a screenshot — Orca's attach path is clipboard paste, which I could not
    drive without synthesizing a clipboard event.

…cope

Every chat row renders through one of the two lists, so build both once instead
of allocating a fresh array per CommentMarkdown instance.
@brennanb2025

brennanb2025 commented Aug 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Approach + readiness pass (re-derived)

Approach check

Rendering user prose through a CommonMark pipeline. Where CommonMark's reading of user text diverges from user intent, the correct repair is a custom remark plugin, not a rewrite of the source string — the same position remark-gfm occupies in this pipeline (remark-gfm + remark-breaks, optional raw-HTML rehype). Selecting between two module-level plugin arrays on a boolean prop is the shape this PR now uses; 2b0a4aaa5a adopts that, so both lists are module constants instead of a per-instance allocation.

Disabling the definition construct is specific to rendering prose a human typed. On assistant output a link reference definition is legitimate, which is why this is not a general-purpose change to the renderer. Verdict: ALIGNED. The mechanism (a remark plugin contributing a micromark extension via micromarkExtensions) is the documented extension point, not a workaround.

Attachment placeholder markers. The [Image <n>] placeholder Orca parses is numbered per prompt being composed — restarting at 1 each turn, not session-cumulative. The bound this PR uses (strip at most imagePaths.length, first-N by document position) is therefore safe under both numbering schemes. [Image #1] Take a look at this image… is also a documented convention users are told to type, so a user typing the marker literally is a realistic input, not a contrived one. Bounding the strip count at all is stricter than the obvious implementation, which strips globally — the tradeoff is deliberate and in the safe direction.

Optimistic echo reconciliation. The materially simpler design is to carry a clientMessageId through the agent protocol so the optimistic bubble reconciles by id and no content matching is needed. That is not available to Orca: it requires owning the wire protocol end to end, whereas Orca reconstructs the transcript from a third-party coding agent's session log and has nowhere to inject an id the agent would echo back. So the normalized-content-key + occurrence-ordinal + message-id-boundary scheme is DIVERGENT-BUT-JUSTIFIED, architectural rather than a shortcut. Not something to change in this PR.

Readiness checklist

# Area Verdict
01 SSH / remote / network Pass. No network surface. normalizeImageTranscriptMessages runs only in src/renderer/ and mobile/src/session/ — the host publishes raw transcript rows unchanged, so no RPC param, stream opcode, or published-content change. Mixed-version pairs see identical wire traffic; an older mobile build simply keeps its own older rendering.
02 Crash / cast / retry / unbounded growth Pass. No new retry or state machine. The three added entry.images!.length assertions sit behind an if (!entry.images?.length) continue guard in the same loop. Pending collections stay capped (8 sends, 32 previews, 8 sessions) and the LRU slices keep the newest entries.
03 Security / supply chain Pass. No dependency, lockfile, shell, eval or network change. rehype-sanitize is untouched and still runs; disabling a parser construct produces less link resolution, never more markup.
04 Performance Pass, net improvement. The removed escape ran a regex pass over every user turn's text on every render; the replacement is a module-constant plugin list and no string work at all. normalizeImageTranscriptMessages is a single O(n) pass that returns its input array unchanged when nothing folds, so downstream memo identity survives; the mobile reconcile callers run it inside effects, not render bodies, and foldMobileNativeChatMessages is memoized on session.messages. Residual: pendingSendsAsMessages remains O(pending x messages) with pending capped at 8 — pre-existing shape, slightly larger constant.
05 Functional correctness Pass. See the measured table and screenshots in the previous comment; 29 behavioural tests go RED on a production-only revert.
06 Back-compat / data loss Pass. Nothing persisted, no schema, no migration, no user data written or deleted.
07 Cross-platform Pass. No paths, no metaKey, no platform branch. CRLF is now a non-issue on the render path because the source string is no longer rewritten; the marker regexes use [^\S\r\n] for horizontal whitespace.

Merged-tree check: main moved to e16a22e during this pass. Its changes touch mobile session-tab and
terminal-handle-recovery files with zero overlap with this PR. I materialized the merge locally and ran it
rather than trusting a clean merge-tree: desktop 1364 passed, mobile 512 passed, web typecheck clean.
Merge aborted afterwards; the branch is unchanged.

Verdict: no P0 or P1. Ready to merge.

GFM adds a second definition construct. `[^note]: I meant this literally` was
still rendering an empty bubble on a user row — the same total content loss,
one construct over — so disable `gfmFootnoteDefinition` alongside `definition`.

The comment claimed reference links survive. Measured, they do not: they resolve
*against* the definitions this removes, so `[d]`, `[d][]` and `![alt][d]` now
render literally. That is the right reading for prose, but the comment said the
opposite. Also pin that the untouched-content cases render something, since
comparing the two prop values to each other is trivially true if the plugin
ever stops attaching.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Correcting my earlier verdict: three P1 regressions, verified

My previous comment said "no P0 or P1, ready to merge." That was wrong. A later review round surfaced
findings I have now reproduced myself against HEAD. All three are regressions this PR introduces versus
bf6dc6f, and all three share one root cause: the fold treats "no visible image-source run" as proof the
markers are the user's own words.
That inference is right for the ticket's case and wrong whenever the
source run exists but is not visible.

P1 — the read window trimming a source turn leaks raw markers into an already-read message

boundNativeChatWindow is a hard count slice — messages.slice(messages.length - limit)
(src/shared/native-chat-merge.ts:49) with NATIVE_CHAT_INITIAL_LIMIT = 300, re-applied on every append
(:90). The head is trimmed one message at a time, so the boundary passes through every image run as a
session ages past 300 turns. Measured at HEAD:

window contents HEAD base
[Image: source: /tmp/b.png] + [Image #1] what do you see image-ref + "what do you see" same
source trimmed, prompt kept "[Image #1] what do you see" "what do you see"
2-image run, one source trimmed image-ref + "[Image #2] compare these" "compare these"

The user's own already-read message visibly gains agent-internal markup as it scrolls into history.

The obvious fix is unsound — I checked. "Spend an unbounded budget when the marker turn is at window
index 0" re-breaks this very ticket: a user whose first message is a literal [Image #1] gets it stripped
again. The ambiguity is genuinely unresolvable from the window alone; distinguishing them needs out-of-window
information (e.g. whether older history exists), which normalizeImageTranscriptMessages does not receive.
Flagging for a decision rather than patching it blind.

P1 — glue matching is now blind to any transcript row carrying an image

advancedNativeChatUserTexts and matchingNativeChatUserTexts return null for a row with image-ref
blocks (native-chat-pending-occurrence.ts:114-116, 134-136); base used
normalizedNativeChatUserMessageText unconditionally. selectPendingIndicesRepresentedByUserTexts also
filters image pendings out of open (:203-205). Measured on a folded turn
[image-ref /tmp/a.png, text "look at this please"] with pendings {"look at this", +image} and
{"please"}:

matchingUserTexts: []   advancedUserTexts: []   representedIndices: []

Base produced ["look at this please"] and represented both. Failure: the host's 500 ms body/Enter gap
loses the Enter, the agent receives one line, the transcript carries one folded turn — neither exact key
matches and the glue fallback can no longer see it, so both optimistic bubbles persist indefinitely, one
of them a visible duplicate of the user's photo. Only pane close or the 8-entry LRU clears them.

P1 — a marker-only echo with a bound local preview renders a literal [Image #1] above the photo

This PR rewrites the assertion at mobile/src/session/mobile-native-chat-render-data.test.ts:176, renaming
restores the local preview onto a marker-only transcript turn to …without treating it as proof to strip marker text and changing the expectation from [{image-ref}] to
[{text:'[Image #1]'}, {image-ref}]. Marker-only turns are the documented normal echo shape for some hosts
(mobile-native-chat-draft-reconcile.ts:179-180), so on those hosts every photo the user sends now renders
with an [Image #1] caption they never typed.
Steady state, not window-dependent. This one looks
deliberate, so it needs an explicit product call rather than a patch.

Also reported, not independently reproduced by me

A mobile ack-lost image-only send whose echo is an empty user turn is never indexed as an image echo
(mobile-native-chat-draft-reconcile.ts:270 gates on imageCount > 0, and an empty turn normalizes to
blocks: [] so its evidence count is 0), while the sibling path at :210 still accepts exactly that shape —
so the preview binds while the send stays held and the deadline fires a false "delivery unknown".

Fixed and pushed in 4cd957976e

  • GFM adds a second definition construct. [^note]: I meant this literally still rendered an empty
    bubble — same total content loss, one construct over. Now disables gfmFootnoteDefinition too.
  • My own doc comment and commit message claimed reference links survive. Measured, they do not: [d],
    [d][] and ![alt][d] all render literally now, because they resolve against the definitions being
    removed. Correct behaviour for prose; the comment said the opposite. Fixed.
  • The untouched-content cases compared the two prop values to each other, which is trivially true if the
    plugin ever stops attaching. Added a .not.toBe('') pin. Mutating the plugin to a no-op now turns
    12 of 20 tests RED.

Desktop 1365 pass, web typecheck and lint clean.

…arkers

The marker fold inferred "no visible `[Image: source: …]` run before this
prompt ⇒ the `[Image #N]` markers are the user's own words". That is right for
STA-4363 and wrong wherever the source run exists but cannot be seen, which
regressed three paths.

Read window. `boundNativeChatWindow` is a hard count slice re-applied on every
append, so an aging session's head cuts through image runs and an already-read
turn gained a literal `[Image #1]`. Only the head is ambiguous — anywhere else
the run would be visible above the prompt — and the paging signal that resolves
it already exists as `hasMore`, it was simply never plumbed. The fold now takes
`hasEarlierHistory` and, at index 0 only, spends an unbounded marker budget: a
partial run's visible image count is a floor, not its real size. Untruncated
windows keep the ticket's rule, so a first-message literal marker still stands.

Glue. Both user-text matchers returned null for any row carrying an image, so a
folded image turn was invisible to the lost-Enter glue path and both optimistic
bubbles — one a duplicate photo — persisted until pane close. They now yield the
row's text with an image-count budget, which keeps the guard that a text-only
row can never retire a send whose photo has not landed.

Mobile render. A marker-only host echo captioned the user's own photo with a
literal `[Image #1]`. A bound local preview is positive proof the turn echoes an
image sent from this device, so its markers are placeholders; stripping is
bounded by the images the turn actually shows.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 832cb300-3f2c-4bf8-976b-d5a05805c991

📥 Commits

Reviewing files that changed from the base of the PR and between 4cd9579 and 445ba46.

📒 Files selected for processing (12)
  • mobile/src/session/MobileNativeChatOverlay.tsx
  • mobile/src/session/mobile-native-chat-image-transcript-markers.ts
  • mobile/src/session/mobile-native-chat-render-data.test.ts
  • mobile/src/session/mobile-native-chat-render-data.ts
  • src/renderer/src/components/native-chat/native-chat-live-message-preparation.ts
  • src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts
  • src/renderer/src/components/native-chat/native-chat-pending.test.ts
  • src/renderer/src/components/native-chat/native-chat-session-assembler.ts
  • src/renderer/src/components/native-chat/use-native-chat-assembled-messages.ts
  • src/renderer/src/components/native-chat/use-native-chat-live-session.ts
  • src/shared/native-chat-image-transcript-markers.test.ts
  • src/shared/native-chat-image-transcript-markers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • mobile/src/session/mobile-native-chat-image-transcript-markers.ts
  • mobile/src/session/mobile-native-chat-render-data.test.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 0 remain after this review.

Comment thread mobile/src/session/mobile-native-chat-render-data.ts Outdated
Comment thread src/shared/native-chat-image-transcript-markers.ts Outdated
…path

The fold's own tests pass the option directly, so a plumbing break anywhere
between the session hook and the fold would leave them green while the fix
did nothing in the app. Covers the mixed-source reassembly branch, which
re-assembles instead of returning the first pass and so needs the signal
handed on separately.
Review found the image budget was read from nativeChatUserMessageImageEvidenceCount,
which counts literal `[Image #n]` text as image evidence. That is correct for
content keys, but as a glue budget it let a row showing no photo retire a send
that carried one: pendings {'a' + photo} and {'b [Image #1]'} against the
text-only row 'a b [Image #1]' scored a budget of 1 and consumed both, erasing
the user's preview before the photo landed.

The budget now counts image-ref blocks only. Under-counting costs at most a
lingering duplicate bubble; over-counting loses the photo.
…udget

Self-review caught the render-time strip re-breaking the very ticket it sits
in. The fold already spends one marker per real `[Image: source: …]` turn and
deliberately keeps any surplus as the user's own words. Charging the bound
preview for those same images spent the budget twice: a turn folded to
image-ref + "compare with the [Image #2] I mentioned" rendered as
"compare with the  I mentioned", deleting text the user typed.

The budget now counts only path-less image blocks — the previews appended for
a marker-only echo, which no source turn ever vouched for.
Self-review caught a regression in the round-1 glue change. Glue matches a
LEADING run of pendings, so admitting image-carrying sends into the candidate
list put them in that prefix. A photo-less transcript row can never consume an
image send, so an unrelated queued photo stranded every text echo behind it:
pendings {'photo caption'+photo, 'a', 'b'} against the row 'a b' retired
nothing, where before the change it retired both text echoes.

A row now only considers image sends when it has image evidence of its own.
The budget check still bounds how many it can then own.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

The three P1 regressions are fixed, plus three more this round found in the fixes themselves

I reproduced all three P1s from the earlier review independently before changing anything, then fixed all
three. The review loop that followed found three further regressions introduced by those fixes — one
from a reviewer, two from my own stress-testing. Everything below is fixed and pushed.

Reproduced first (measured at 4cd957976e, before any change)

case HEAD then base bf6dc6f
source turn trimmed out of the window "[Image #1] what do you see" "what do you see"
2-image run, one source trimmed "[Image #2] compare these" "compare these"
glue matchers on a folded image row matching [], advanced [], represented [] ["look at this please"], both represented

boundNativeChatWindow confirmed a hard count slice: 302 messages in, 300 out, head at index 2.

P1-1 — window trim. The signal that resolves it already existed

The earlier review concluded the ambiguity "needs out-of-window information which
normalizeImageTranscriptMessages does not receive". That is true of the function but not of the app: the
transcript read already tracks hasMore, it was simply never plumbed down. It is now a hasEarlierHistory
option that defaults to the previous behaviour.

The key narrowing is that only window index 0 is ambiguous. A marker turn anywhere else would have its
source run visible above it, so its absence there really is evidence the user typed the markers. So at index
0 only — and only when older history exists — the fold spends an unbounded marker budget, because a partial
run's visible image count is a floor rather than its real size.

This is strictly better than both sides of the old trade. Untruncated window (the ticket's own scenario, a
short session) keeps markers verbatim, so STA-4363 stays fixed; a long session no longer leaks
[Image #1] into a message the user already sent. The residue is narrow: a literal marker sitting exactly at
the head of a truncated window is still stripped, which is base behaviour, and only while it occupies that
one slot.

Plumbed through use-native-chat-live-session → use-native-chat-assembled-messages →
native-chat-live-message-preparation → native-chat-session-assembler, and on mobile through
MobileNativeChatOverlay → mobile-native-chat-render-data. hasEarlierHistory is only applied to the
transcript source — hasMore describes transcript pagination, not scrollback.

P1-2 — glue

Both user-text matchers returned null for any row carrying image blocks, so a folded image turn was
invisible to the lost-Enter path and both optimistic bubbles — one a duplicate photo — persisted until pane
close. They now return {text, imageCount} rows, and the matcher admits image-carrying sends bounded by the
row's image evidence, so a text-only row still can never retire a send whose photo has not landed.

P1-3 — mobile render. This one is a bug, not a product call

I disagree with the framing that this needs an explicit product decision. findLandedImagePreviewEchoes
binds a local preview only to a turn that provably replaced an optimistic image send — that is precisely
the out-of-band evidence the fold was missing. A bound preview proves the markers are placeholders, so
captioning the user's own photo with a literal [Image #1] is never the right render. The strip is bounded
by the images the turn actually shows.

Then the review loop found three regressions in those fixes

  1. Marker text inflated the image budget. The row budget read
    nativeChatUserMessageImageEvidenceCount, which counts literal [Image #n] text as image evidence —
    correct for content keys, wrong as a budget. Pendings {'a' + photo} and {'b [Image #1]'} against the
    text-only row 'a b [Image #1]' scored a budget of 1 and consumed both, erasing the user's photo
    preview before it landed
    . Budget now counts image-ref blocks only. Under-counting costs a lingering
    duplicate; over-counting loses the photo.
  2. The preview strip double-spent the fold's budget. The fold spends one marker per real source turn and
    deliberately keeps any surplus as the user's own words. Charging the preview for those same images spent
    it twice: a turn folded to image-ref + "compare with the [Image #2] I mentioned" rendered as
    "compare with the I mentioned" — deleting text the user typed, i.e. re-breaking this very ticket at the
    render layer. Budget now counts path-less blocks only.
  3. Image sends blocked the glue prefix. Glue matches a leading run, so admitting image sends into the
    candidate list let an unrelated queued photo strand every text echo behind it: {'photo caption'+photo, 'a', 'b'} against the row 'a b' retired nothing where base retired both. The open filter is now
    row-aware.

Evidence

Every fix has a test that goes RED when only the production change is reverted (signatures kept, so no
failure is a compile artifact): 9 RED across the six fixes — 2 window-trim, 2 glue, 2 preview-strip,
2 end-to-end plumbing, 1 budget, 1 prefix. A further 5 tests are guards that stay green both ways by design;
they pin that the fixes did not over-strip or over-loosen, and are called out as such rather than counted as
evidence.

The plumbing tests matter on their own: the fold's unit tests pass the option directly, so a break anywhere
between the session hook and the fold would have left them green while the fix did nothing in the app. They
cover the mixed-source reassembly branch, which re-assembles rather than returning the first pass and so
needs the signal handed on separately.

Desktop native-chat 975 pass; full mobile suite 449 files / 3545 pass, 0 failures; both typechecks clean;
oxlint clean and verified live by injecting a violation. None of the ~45 tests encoding prior incidents
turned red at any point. Four failures in the full desktop suite (relay/subprocess, orca-runtime
worktree-scan, two WorktreeCard suites) are pre-existing — I confirmed one still fails on 4cd957976e
with none of this work applied — and none of them import any changed module.

Wire compatibility

No new field, no new opcode, and no change to what the host publishes — hasMore is already negotiated and
this only changes how the client renders data it already receives. Both desktop and mobile normalise it with
a ?? messages.length >= limit fallback, so hasEarlierHistory is a real boolean even against an old host,
degrading to the length-based inference rather than to undefined.

@brennanb2025

brennanb2025 commented Aug 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Visual proof — desktop native chat, real dev instances on both commits

Two isolated Orca dev instances, same synthetic Claude transcript, same steps. The only difference is the commit each instance was launched from:

commit dev instance identity
before 4cd957976e (= 445ba46ad1^) Orca: 4cd957976e
after 2719292973 (branch head) Orca: brennanb2025/rev-14939

The fixture. A 311-record Claude JSONL transcript. Chronologically: 10 warm-up turns, then a [Image: source: /Users/brennan/Desktop/dashboard.png] user turn, then the prompt it belongs to — [Image #1] what do you see in this screenshot? — then a short visible tail. The desktop read window is a hard 300-message slice, so it cuts between the source turn and the prompt: the anchor is trimmed away and the prompt becomes the window head. The tail is padded with <system-reminder> turns, which count toward the 300 but are stripped from the rendered list, so the whole visible conversation fits one screen while the window is still genuinely trimmed. The "Load earlier messages" control at the top of each screenshot is the rendered proof that hasMore is true.

The fixture also carries a second user turn that literally types [Image #1] hello with no image behind it — the ticket case. It must stay verbatim in both builds.


Before — 4cd957976e

The already-sent prompt shows a raw [Image #1] the user never typed.

before-14939.png

After — 2719292973

Same transcript, same window. The head prompt reads as the user wrote it, and the literally-typed [Image #1] hello two turns below is untouched.

after-14939.png

After — 2719292973, with "Load earlier messages" clicked

This is the frame that shows the fold is not just deleting markers. Once the window grows and the trimmed [Image: source: …] turn comes back into view, the same turn re-renders as a dashboard.png attachment with clean text — so the marker the previous frame dropped really was standing in for an image. [Image #1] hello is still verbatim.

after-paged-14939.png


How the panes were bound. Each dev instance ran on its own ORCA_DEV_USER_DATA_PATH profile with the Chat UI experimental setting enabled through the real settings action, a throwaway git project added, and the workspace opened normally. The pane was switched to chat with the pane header's Show chat view control. The one thing injected was the agent-status entry the Claude hook would otherwise publish — agentType: 'claude' plus a providerSession carrying the transcript path — through the renderer's own setAgentStatus action, i.e. the same seam the hook IPC writes to. Everything downstream of that is production code: nativeChat:readSession windowing, hasMore, the marker fold, and the message list.

Branch moved after capture. The head advanced to 0f93e9511a while these were being taken. git diff 2719292973 0f93e9511a touches nothing on the marker-fold path — native-chat-image-transcript-markers.ts, native-chat-live-message-preparation.ts, use-native-chat-live-session.ts and use-native-chat-assembled-messages.ts are byte-identical — so the frames still describe the current head. The three newer commits are pending/glue-budget and mobile render-data changes.

What this does not cover. No live agent was attached, so streaming, sends, and optimistic-echo pruning are not exercised here — the pending-occurrence image-budget change in this PR is covered by unit tests, not by these frames. Mobile is not covered.

A base-vs-head differential over eight glue shapes found three more defects.

GAP MATCHING (a regression my own previous commit introduced, and one the PR
already had in another form). Skipping a send the row cannot afford let glue
match a NON-CONTIGUOUS subsequence: sends 'fix', 'the'+photo, 'bug' against a
row reading 'fix bug' retired 'fix' and 'bug' even though a third send landed
between them, so a bubble vanished while its send was still in flight. Base
never did this. Every unretired send is a candidate again, in send order, and
the budget rejects unaffordable groups instead of stepping over them.

MARKER-ONLY HOST ECHOES. Some hosts echo an image send as bare '[Image #n]'
text with no '[Image: source: …]' turn, so the markers are the only evidence
the photo landed. Reading such a row literally matched nothing and left both
optimistic bubbles — one a duplicate photo — on screen forever. A row now
carries a second identity for that case, tried only after the literal reading
fails, so a marker the user actually typed still matches verbatim first.

Differential vs bf6dc6f now agrees on seven of eight shapes. The eighth is a
deliberate improvement: base retired a photo-carrying send against a row
showing no photo, erasing the preview before it landed.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Heads up: this PR is CONFLICTING against main, so CI is silently not running

gh pr checks reports "no checks reported on the branch" — not "pending", not "failing". None of the recent
commits have been validated by CI. The PR was MERGEABLE earlier today; main has since moved 66 commits.

One conflicting file: mobile/src/session/use-mobile-native-chat-drafts.ts. It is not from the
marker-fold work — it is this PR's existing mobile draft changes meeting a concurrent refactor on main.

I aborted the merge rather than resolve it, because it is not mechanical:

  1. The auto-merge silently drops a re-export. main added
    export { normalizeNativeChatUserText as normalizeReconcileText } to
    mobile-native-chat-draft-reconcile.ts. That file auto-merges without conflict and loses the line,
    because this PR edited the same import/export region. use-mobile-native-chat-drafts.ts on main imports
    normalizeReconcileText, so a "keep both sides" resolution of the visible conflict produces a broken
    import that the conflict markers never point at.
  2. main extracted the retirement logic into new modules — mobile-native-chat-pending-retirement.ts
    and mobile-native-chat-pending-baseline.ts — replacing the inline filter this PR modifies.
    retireLandedMobileNativeChatPending has no image awareness, so this PR's imageCount /
    countImageSourceTurnsAfter semantics have to be folded into the new module rather than kept alongside it.
  3. The baseline record conflicts semantically: this PR adds imageCount and a 3-argument
    countUserTextOccurrences; main adds baselineResolved: transcriptSettled. Both are needed, so it is a
    merge of intent, not of text.

Resolving this well means understanding both sides of a reconciliation module that has a history of
incidents, and getting it wrong fails quietly — echoes that never retire, or previews retired before their
photo lands. That is a different piece of work from the marker-fold fixes and I did not want to guess at it
inside this PR without it being an explicit call.

Worth knowing: the visual proof above was captured off the branch, not off a merged tree. If the conflict
resolution touches the fold path, those frames need retaking.

Both mobile hops cast the wire payload without validating per-field types, so
a host sending a non-boolean `hasMore` decided paging: `result.hasMore ?? …`
yields a truthy string unchanged, and `applied.hasMore != null` accepts it.
The desktop runtime parser already rejects this; mobile is the half that never
got the guard.

That mattered more once the marker fold started reading this signal, because
the fold changes what a message says. The fold itself is already safe — it
gates on an explicit `=== true` — so this closes the remaining exposure: the
load-earlier affordance and the declared `hasMore: boolean` contract.

A non-boolean now reads as "the host did not answer", which falls through to
the count inference exactly as an older host does.
…erence

Gating the marker fold on the reported answer was defeated on the first
snapshot frame. Both remote adapters build that frame as
`hasMore: frame.hasMore ?? messages.length >= limit`, so an inference is baked
in before the hook sees it and `hasMore === true` cannot say who decided. Only
the first snapshot is affected — the later-snapshot and replacement branches
use `?? false` — but that is the frame that lands on first paint.

The effect was the flip this gate was meant to prevent, in the unsafe
direction: against a host predating the field the read seed correctly leaves
the fold off, then the snapshot turns it on, and a marker the user typed
disappears a beat after the pane renders.

Frames now carry the host's answer separately from the value the affordance
uses. Every host-derived construction sets it — this process computes it, so
it always reports; the adapters pass it through only when the host actually
sent a boolean. Absence therefore means "unanswered" on every path, and the
fold reads that instead.

Additive optional field on an IPC payload and on the runtime frame, so an
older host simply omits it and the fold stays off, which is what base did.
…antees

The comment claimed the flag is true only when the host said so. A replay that
invalidates the cursor synthesizes it too, and that is correct — a bounded
window which dropped its oldest row really does have history behind it. What
the flag actually excludes is a count standing in for knowledge, which is the
distinction the marker fold depends on.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review loop closed — and it ends on a blocker. Please do not merge yet.

The three filed P1s are fixed. The five review rounds that followed found eleven further defects, seven of
them introduced by the fixes themselves
. Every round found something in the previous round's work,
including the last one. One of those findings is still open and I am recommending against merge until it is
resolved.


BLOCKER — one transcript row retires two sends, and the user's photo is lost

src/renderer/src/components/native-chat/native-chat-pending-occurrence.ts:49-73

nativeChatUserMessageContentKeys returns a set of keys, and both count-builders credit +1 to each.
So one user turn vouches for two different pending sends. A row with markers, no image-ref block, and
non-empty stripped text — [Image #1] fix — mints both:

  • text:[Image #1] fix (the literal reading — this ticket's own premise)
  • text:fix\0images:1 (the marker-as-image-evidence reading)

The second is exactly the key a photo-carrying send owns. So a row that renders no photo retires a send
that carried one.

A sequence a user can actually produce: type [Image #1] fix literally, then send a photo captioned fix
before the first turn lands. The first turn lands with no [Image: source: …] above it; the assistant
replies. Measured against bf6dc6f:

base head
pending kept [photo send] []
photo rendered anywhere yes no

prunePendingSends drops it from the cache once the assistant turn lands. If the photo's own turn never
materialises — send failed, host stalled, transcript rotated — the photo and its caption are gone with no
trace. On the happy path the preview still blinks out in between.

A 65,016-case differential found 108 cases where head loses a photo base showed, and zero the other way.

Why I have not fixed it. Both readings are individually legitimate: dropping the marker-derived key
turns 4 tests red, because it encodes the paginated-window case where markers are the only evidence a real
image send landed. The bug is crediting both from one row, which is a matching constraint — one row must
satisfy at most one pending. The consumers recompute counts per-pending against different boundaries, so
there is no shared place to record that a row is spent. That is a real change to the occurrence arm, which
is the most incident-encoded code in this area, and seven of my "contained" changes here were wrong this
session — each caught only by the following round. Patching it blind is likelier to add a P1 than remove one.

Also worth knowing before scoping it: the glue arm loses a photo the same way (row [Image #1] fix bug,
sends ["fix", photo+"bug"]), but base does too, so it is not a regression. A content-key-only fix will not
cover it.


The three filed P1s

All three reproduced independently against 4cd957976e before anything changed.

Window trim. The earlier review concluded the ambiguity needed information the fold never receives. The
signal existed — the read already tracked hasMore, it had just never been plumbed. The narrowing that makes
it safe: only the oldest row of the paginated window is ambiguous, because a marker turn anywhere else
has its [Image: source: …] run visible above it. An untruncated window keeps markers verbatim, so
STA-4363 stays fixed.

Glue. Both matchers returned null for any row carrying an image, so a folded image turn was invisible
to the lost-Enter path and both bubbles — one a duplicate photo — persisted until pane close.

Mobile render. Reported as possibly a product call; it is a bug. A bound local preview is proof the turn
replaced an image send, which is exactly the evidence the fold was missing.

What the loop found in those fixes

  1. Marker text inflated the image budget, so a photo-less row retired a photo-carrying send.
  2. The preview strip double-spent the fold's budget, deleting text the user typed — re-breaking this
    ticket at the render layer.
  3. Image sends blocked the glue prefix, stranding every echo behind them.
  4. Gap matching: a fix of mine rested on a false premise about base, and retired echoes whose sends were
    still in flight.
  5. The textless barrier blocked runs it did not separate, permanently.
  6. Marker-only host echoes stopped matching, where base matched them.
  7. The window-head rule keyed off list position, which is a scrape row or the smallest id once lists are
    merged and re-sorted.
  8. Naming it by id dropped a guard, so undefined === undefined fired the rule with no head at all.
  9. The mobile preview budget counted path-less blocks rather than what was appended; some agents emit an
    inline image as a url with no path.
  10. The paging gate rode a count inference; the reported answer is now kept distinct end to end.
  11. Mobile never type-guarded the host's paging answer, where desktop does.

Evidence

Every fix has a test that goes RED when only the production change is reverted, signatures preserved so no
failure is a compile artifact — 26 RED in total. Guards that stay green both ways are counted as guards,
not evidence.

Base-vs-head differentials through the real pipeline: 27,720 cases (round 4) and 572,760 (round 5), with the
base copies running against base markers. Head's retire set is a strict subset of base's — confirmed
structurally and then independently by exhaustion over 32,760 combos with 956 divergences and zero head-only
retires. The single family where head retires more is the ticket itself.

Desktop 1005 pass, mobile 1229 pass, both typechecks clean, lint clean and verified live by planting a
violation. None of those tests catches the blocker above — that is the honest summary of what a green
suite was worth here.

Corrections to the record

C3 is narrower than filed and is not being fixed. The fold strips markers by document position and
never reads the ordinal, so the case as originally described does not survive the fold at all — it needs a
host emitting a surplus marker. A proposed fix was implemented in scratch and rejected: a no-op for
the filed case, and it introduced a steal where an earlier row consumes a prefix a later row owns.

The hasMore over-estimate was fixed by another commit on this branch, which takes the signal from the
host and infers only for a host predating the field.

Follow-ups, scoped, not fixed here

  • Mobile carries an unfiled instance of P1-2. Both retirement arms miss: the glue arm makes an
    image-carrying send a null segment by design, and the preview arm matches the whole row's text, so a
    glued row never equals the photo send's caption. Needs a captioned photo send plus an adjacent text
    send whose echo is one glued row; caption-less and own-row sends are unaffected. The preview arm is the
    cheaper half.
  • Desktop has no glue slide — one unretirable send disables glue retirement for the rest of the session.
    Mobile solves this with a slide budget. Pre-existing.
  • Desktop glue has no per-send boundary, so a row can retire a send issued after that row existed.
  • A bound preview replaces a host image's url rather than appending beside it, so a different image can
    render. Pre-existing, now pinned by a test.
  • The mobile MAX_MESSAGES clamp forces the paging flag false at 2000 rows. Harmless for the fold now,
    but it means the load-earlier affordance disappears at the cap.

What was not established

The screenshots exercise the window-trim fix only — no live agent was attached, so streaming, sends and
optimistic-echo pruning are not in those frames, and the glue and budget work rests on unit tests. Mobile has
no visual coverage. The frames were captured off the branch, not a merged tree.

The differentials are exhaustive over a hand-picked alphabet and transcript/pending runs of length ≤3-4, not
over all inputs. The staleness sweep is code-path reasoning plus the repo's hook tests, not a measurement
under a real renderer. The mobile deepest-window clamp was read, not run.

…s type

The local handler was annotated `Promise<ReadTranscriptResult>`, which has no
`hasMore`. The field reached the renderer anyway — a returned call result is not
a fresh object literal, so excess-property checking never looked at it — but the
type said otherwise, and nothing type-links this result to the preload shape that
now reads it. Any later refactor that built the result inline would have dropped
the flag silently, and the renderer would have fallen back to inferring paging
from the row count: the exact defect the flag exists to close.

Covered end-to-end from a real on-disk transcript, including the case the count
inference cannot express — a window that is exactly full reports `hasMore: false`.

Also pins, at the call sites a pure-function test cannot reach:

- the window-head marker rule through the live session hook, so a call site that
  stops forwarding the host's paging answer fails rather than silently reverting
  to "older history always exists"
- the two rules re-homed onto #14936's modules: a marker-only send that carried
  no attachment still earns a real boundary, and a photo turn whose caption
  happens to spell a run cannot retire the typed sends around it

Mutating each re-homed line turns 3, 3 and 4 tests red respectively, so the merge
landed live code rather than dead code.
…aging flag

The window-head cases were driving `hasMore`, which the overlay no longer reads.
`hasMore` still merges the host's answer with a count inference that reports true
for a window filled to exactly the limit; the fold rides `earlierHistoryConfirmed`,
which is set only when the host actually answered.

Pins that separation at the call site a pure-function test cannot reach: an
inferred paging answer the host never confirmed must leave a head marker alone.
Riding the inference there erases an `[Image #n]` typed at the true start of the
conversation — this ticket's own defect — and mobile's 40-turn window makes the
exact fill reachable. Pointing the overlay back at `hasMore` turns 2 tests red.
Three subscribe-lifecycle cases assert the whole snapshot frame with
toHaveBeenCalledWith, so adding `hasMoreReported` to it turned them red on the
branch head — an exact-shape assertion fails on any added field, including a
deliberately optional one.

The window-head cases also still drove `hasMore`, which no longer gates the fold;
they now emit the reported field the fold actually reads, so the case cannot pass
on a value the host never answered.
… image send

Splitting echo lookup into literal and image maps, and requiring the row's
evidence count to cover the send, dropped a shape the base matched: a user turn
the host echoed with no content at all. Its evidence count is 0, so an image-only
send could never claim it.

Measured against the merge base, which used one map keyed by
`normalizedUserText(message) ?? ''` with no count requirement at all:

    base  imageCount 1 vs empty echo -> landed
    HEAD  imageCount 1 vs empty echo -> not landed   (imageCount 0 still lands)

So this branch introduced it; it is not inherited. The user-visible end is a send
that stays held while the sibling preview-binding path — which accepts exactly
this shape — has already put the photo on screen, until the 20s deadline warns
"delivery unknown" about a message that plainly landed.

An empty turn testifies to no particular number of images, so it now satisfies
any count rather than being read as evidence of zero. It still keys under '',
which is what keeps the reach narrow: only a captionless send can match it, and a
captioned one is still refused because the turn says nothing about a caption.
That is the base's own reach, restored, rather than a wider one.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Correction: the empty-echo case is introduced by this PR, and is now fixed

In my previous comment I filed the mobile ack-lost image-only send as a follow-up and said it was
"pre-existing, not introduced here." That was wrong, and a later review round caught it. I have
since measured it both ways.

At the merge base, findLandedUnconfirmedSends used a single map keyed by
normalizedUserText(message) ?? '' with no image-count requirement at all, so a user turn the
host echoed with no content matched a captionless image send. Splitting that into
literalMessagesByText / imageMessagesByText and requiring message.imageCount >= entry.imageCount
dropped the shape, because an empty turn's evidence count is 0:

base   imageCount 1  vs empty echo -> landed
HEAD   imageCount 1  vs empty echo -> NOT landed      (imageCount 0 still lands)

The user-visible end: the send stays held while the sibling preview-binding path — which accepts
exactly this shape — has already put the photo on screen, until the 20s deadline warns "delivery
unknown" about a message that plainly landed.

Because it is a regression this branch introduces rather than something inherited, it belongs here
rather than in a follow-up, so it is fixed: an empty turn now satisfies any image count instead of
being read as evidence of zero. It still keys under '', which keeps the reach narrow — only a
captionless send can match it, and a captioned send is still refused, because the turn says nothing
about a caption. That is the base's own reach restored, not a wider one.

I had earlier rejected a candidate fix on the grounds that it let a whitespace-only turn claim a
two-image send. That objection was misplaced: the base did the same thing, since it had no count
check at all. Reintroducing a hazard the base already had is not the same as adding a new one, and
the alternative is a false "delivery unknown" on every ack-lost image send on those hosts.

Also worth recording from the same round, since both were gaps in my own work:

  • Two lines re-homed by the main merge were dead — mutating them turned nothing red
    (mobile-native-chat-pending-retirement.ts, the image-row exclusion in selectGluedPendingIds;
    and mobile-native-chat-pending-baseline.ts, the literal-marker key). Both are now covered and both
    go red under re-mutation.
  • The hasEarlierHistory guard was untested on both surfaces: dropping it left desktop and mobile
    fully green, while every conversation that fits in one window would lose a typed [Image #1] from
    its first turn — the single most likely way a user hits this ticket. Now pinned through the real
    hook and the real component, in both directions.

Residual risks recorded, not fixed

  • Mobile head-row flicker on reconnect. Desktop batches its read and paging updates in one
    handler; mobile resets the paging flag on reconnect while the retained transcript stays on screen,
    so a head row's markers can appear and then vanish for the reconnect window. Cosmetic, one row, and
    not staged live.
  • Load-earlier reaching the true start flips the flag with the same head id, so that row's markers
    change from stripped to literal in place. Correct semantics — the evidence genuinely improved — but
    it is a visible change to an already-read message.
  • Mobile's legacy growing-tail read branch ignores the host's paging answer entirely. Untouched by
    this PR.

A row like `[Image #1] fix` reads two ways: the user's own marker text, or a
host echoing a one-image send captioned "fix". The key minter returned BOTH
readings and each count builder credited each, so a single turn vouched for
two different pending sends. The second key is the one a photo-carrying send
owns, so a row rendering no photo retired a send that carried one and
`prunePendingSends` deleted it from the cache. If that send's own turn never
landed the photo and its caption were gone with no trace.

The two readings are byte-identical in the row, so nothing about the row alone
can separate them — but they do not need to be separated there. A send that
carried no image owns the literal reading, so the pending list says which
readings are spoken for, and that set does not vary with the per-send boundary
each count is sliced at. When a send is waiting on the literal reading, that
reading wins and the row stops there; otherwise the marker reading stands,
which keeps a marker-only host echo retiring its send.

The count builders take the pending list rather than a precomputed set, which
keeps both call sites to one line each. The set is small (the queue is capped)
and rebuilt per pending, which is cheaper than it looks and much easier to
follow.

Bounded to the two desktop modules — mobile has its own retirement path and
imports neither — and it leaves every boundary function, the occurrence
bookkeeping and the glue arm untouched.

Known residual, unchanged by this: a text-only send whose text equals a photo
send's caption still resolves to one row. Both readings are defensible there
and base lost the photo too, so it is not a regression.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Blocker cleared — b3e65c5fe0

The P1 I flagged above (one transcript row retiring two sends, losing the user's photo) is fixed. Withdrawing
my no-merge recommendation.

The fix. The two readings of a row like [Image #1] fix are byte-identical in the row, so nothing about
the row alone separates them — but they don't need to be separated there. A send that carried no image owns
the literal reading, so the pending list already says which readings are spoken for, and that set does not
vary with the per-send boundary each count is sliced at. It is consulted at mint time: when a send is waiting
on the literal reading, that reading wins and the row stops there; otherwise the marker reading stands, which
is what keeps a marker-only host echo retiring its send.

This matters because my earlier reasoning for deferring was wrong in a specific way, and the correction is
worth recording: I assumed exclusivity needed consumption-time state — a record that a row had been spent —
and there was nowhere to put it, since the counts are recomputed per pending against different boundaries.
That framing was the obstacle, and it was the wrong framing. The resolver is per-row × whole-pending-list,
and that context is invariant across those slices.

A second correction to the record, because it changes what the marker key is actually for: the
"paginated window" justification for it is unreachable in the real pipeline. Pruning runs against
post-fold messages, so at a true window head the fold has already stripped the markers and no marker key is
minted at all. Measured, same row and same pending, only the head id differing:

keys minted photo send retired
not the window head literal + marker-derived yes
is the window head literal only no

So the marker-derived key fires only on rows the fold has just declared to be text the user typed. What the
existing tests protect is the marker-only host echo — a host echoing an image send as bare [Image #n]
with no source turn anywhere in the window. That case is real and is preserved; the trimmed-window case it
was described as protecting never reaches this code.

Scope and evidence. Two desktop modules; mobile has its own retirement path and imports neither. Every
boundary function, the occurrence bookkeeping and the glue arm are untouched. Two new tests: one pins that a
row cannot retire both sends, one pins that a marker-only echo row still retires a photo send when nothing
claims the literal reading. Neutering the guard turns the first RED and leaves the second green. Desktop
native-chat 1004 pass; pnpm run typecheck:tsc clean; oxlint clean, including max-lines on
native-chat-pending.ts with no disable added.

Still open, unchanged from the summary above: the glue arm loses a photo the same way in one shape, but
base does too, so it is not a regression and a content-key fix does not cover it. Desktop still has no glue
slide and no per-send glue boundary, both of which mobile solves. Mobile still carries an unfiled instance of
P1-2. All four are follow-ups with evidence in the previous comment.

…load-earlier

Taking the host's paging answer for the affordance was too strong in one
direction. A reported `false` is exact and rightly ends pagination. A reported
`true` says older history exists — not that the next read can reach it.

The runtime RPC host clamps the requested window to 2000 turns
(`MOBILE_NATIVE_CHAT_MAX_WINDOW`), so past that it keeps answering `hasMore: true`
while returning the same capped tail. Riding that answer left a "Load earlier"
button that re-read the whole window on every scroll near the top and never grew
it — a regression the count rule alone did not have, since a short read ended
pagination on its own.

So a reported `false` still short-circuits, sparing the wasted read the count rule
costs on an exactly-full window, but a reported `true` has to be confirmed by the
read actually filling the window it asked for.

The fold is unaffected: it never went through this function, and reads the host's
answer directly.
…oad frame

Two more exact-shape assertions broke on the additive `hasMoreReported` field —
the web preload snapshot frame, and (already fixed) the IPC subscribe frames.
Both assert the whole frame, so any added field fails them. Third instance of
this class on the branch; the lesson is that a narrow suite filter hides it,
since neither file sits under a `native-chat` path.

Also drops a redundant `any` constituent the changed-code quality gate caught:
`react-test-renderer` ships no usable types here, so annotating the handle as
`ReactTestRenderer | null` makes the union redundant. Each case now owns its
renderer with an inferred type, which needs no annotation at all.

And corrects the note on literalContentKeysClaimedByPendings: it is not computed
once — each count builder rebuilds it, which is sound because the set does not
vary with the boundary each count is sliced at, and the queue is capped at 8.
…live list

The overlay derived the head from `session.messages[0]`, which slides. Mobile
bounds the merged list to 40 on every live append, so one ordinary append past
the window trims the front and the row that slides up becomes the "head" —
while `earlierHistoryConfirmed` stays true from the initial snapshot, because
the appended path never carries a paging answer.

Measured: a row rendering `keep [Image #1] literal` a moment earlier silently
became `keep  literal`. That is this ticket's own defect, on the oldest visible
row, firing on any agent run past 40 turns.

The head is now recorded where the host actually answers — snapshot, replacement
and the cursor page — and never from an append. When a live trim drops the
recorded row, the id matches nothing on screen and no row is treated as a head:
nothing visible is then known to sit mid-run, so nothing may lose its markers.
Losing the strip for a genuinely trimmed run is the safe side of that trade; the
other side rewrites a message the user has already read.

Desktop needed no change — it already names the head from its read list, which
the append merger copies rather than aliases.

The overlay test alone could not catch this: it drives a fabricated session, so
it only proves the overlay honours the value. Pointing the hook back at the live
head leaves it green and turns the new hook-level case red.
Asserting on `captureSendOrigin.mock.calls[0][0]` against a `vi.fn(() => …)`
declared with no parameters types `calls` as the empty tuple, so the index is
TS2493. The test passes at runtime and mobile `tsc` excludes test files, so
neither CI nor the local typecheck ever saw it; a full type pass does.

Introduced here when the assertion moved off `toHaveBeenCalledWith` to stop
pinning the arity after `captureSendOrigin` gained an images parameter.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review loop closed: round 4 CLEAN with no changes

Four rounds ran. The loop's stopping rule is a round that comes back clean having changed
nothing
, and round 4 is the first one that did — every earlier round found something real.

round verdict what it found
1 no report delivered its one change was reverted (see below)
2 NOT CLEAN two re-homed merge lines were dead (mutating them turned nothing red); the hasEarlierHistory guard was untested on both surfaces; the IPC read result carried the paging flag only by accident of TypeScript's excess-property rule
3 NOT CLEAN the paging fix made the load-earlier affordance immortal past the host's 2000-turn clamp; a third exact-shape frame assertion was CI-red; measured the mobile head-slide
4 CLEAN, no changes full suite, all gates, ~20 mutation probes — every new guard goes red when its production line is reverted

CI is green on the branch: 45 checks pass, 0 fail, MERGEABLE / CLEAN.

Two process findings worth keeping

Every fix in this area had a sibling call site, and the sibling is where the next defect was.
Three times: the image budget guarded the glue arm but not the content-key arm — that became the
P1 that deleted the user's photo; the window-head identity fix guarded desktop but not mobile —
that re-broke this ticket on a row already on screen; and an additive frame field broke exact-shape
assertions in three separate test files. When one arm is fixed here, check its twin before pushing.

Adding a field to a shared frame type has an unbounded assertion blast radius. Three CI-red
instances had one cause: path-filtered test runs. None of the affected files' paths contain
native-chat or nativeChat, so no filter for this ticket reached them. The cheap check is
grepping for exact-shape assertions on the type (grep -rl "nativeChat\|native-chat" src --include=*.test.ts), not choosing a better filter.

Deliberately not fixed, with repros

A stuck photo bubble, as the price of the one-row-one-send guard. Pendings [Image #1] fix
(no image) and fix + a photo, same boundary; transcript echoes both as [Image #1] fix. On this
branch kept = ["p-photo"] — the photo send never retires and renders as a duplicate queued
bubble until eight later sends push it out of the capped cache. Base retired both. Left alone on
purpose: the guard prevents real data loss (the user's photo deleted from the cache with no
trace), the trade is stated in its own comment, and the minimal-looking repair is not
slice-invariant — each count builder runs over a different post-boundary slice, so a photo send
with a later boundary still strands, and a wrong version re-opens the photo-loss path. Reaching it
needs a typed literal [Image #n] and a photo whose caption renders to that same text, on a
host that echoes image sends as bare marker text.

The subscribe hop does not type-guard a non-boolean paging answer. The read hop does. The fold
is safe either way, since it gates on hasMoreReported === true and a non-boolean is absent for
that test, so this is affordance-and-contract only. A fourth transport change this late is the
wrong trade.

Considered and rejected: using the run's known floor instead of an unbounded budget when the
window head is itself a run start. It preserves more user text, but it partially reverts the
window-trim fix — that fix exists precisely because a head run's sources may have been trimmed, so
the visible count is a floor and not the run's size. The two goals genuinely conflict at the head;
the resolution is to restore the base's strip-everything there while preserving user text
everywhere below.

Recorded, not changed: mobile is protected from the immortal-affordance bug only because
MAX_MESSAGES (2000) happens to equal the host's MOBILE_NATIVE_CHAT_MAX_WINDOW (2000). That
coupling is undocumented and would break silently if either constant moved.

Verification

  • Desktop full suite (no path filter): 5868 files / 54852 tests — 22 failures, all in
    pty-*, relay/pty-handler-*, terminal-ime-xterm-*. The identical set run against
    origin/main produces the same 22 in the same 8 files, so they are environmental (Node 26 /
    real $HOME), not this PR's.
  • Mobile: 134 files / 1239 tests pass. Both typechecks clean. oxlint clean, oxfmt clean.
  • Changed-code quality gate: 0 new findings across 51 changed files, all three passes.
  • Every gate was proved live rather than trusted: a planted type error exits 1, a planted lint
    violation is reported, and the desktop typecheck was confirmed to cover src/**/*.test.ts.
  • Non-vacuity: ~20 production lines reverted one at a time, each turning its own guard red. One
    guard I wrote was itself vacuous — an overlay test that drove a fabricated session, so mutating
    the hook left it green. It is now backed at the hook level, and pointing the hook back at the
    live list turns that case red.

Visual proof

Not obtained, and I am not claiming otherwise — this change does have a rendered surface. I built
a dev instance from this branch, attached over CDP and verified the debug port belonged to this
worktree, and created an isolated folder workspace programmatically to avoid a native file dialog.
Reaching a rendered chat row needs a live agent session plus the per-provider chat opt-in, and one
UI interaction mis-targeted and typed into an unrelated editor buffer. I verified that file was
byte-identical on disk with a clean git status, undid the buffer, and shut the instance down
rather than keep driving. So the evidence above is measured rather than visual.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Visual proof — rendered chat rows, two real dev instances on the two real commits

Two isolated Orca dev instances, same synthetic transcript, same steps. The only difference between them is the commit each was launched from.

commit dev instance identity (asserted over CDP in the same block as each capture)
before be07b43a2b (merge base) devRepoRoot = …/vis14939-before
after 13b9f82e37 (PR head) devRepoRoot = …/vis14939-after

Each ran on its own ORCA_DEV_USER_DATA_PATH profile, its own renderer port and its own CDP port. Chat UI was enabled through the real settings action, a throwaway git project was added and opened normally, and the pane was switched with the pane header's Show chat view control. The one thing injected was the agent-status entry the hook would otherwise publish — agent type plus a providerSession naming the transcript path — through the renderer's own setAgentStatus action. Everything downstream of that is production code: session read, the marker fold, and the message list.

The fixture is an 11-record transcript, deliberately shorter than the read window so hasMore is false: there is no Load earlier messages control and no scrolling, so nothing can perturb the frame. It carries four user turns that the fold has to get right.


Before — be07b43a2b

marker-fold-before.png

After — 13b9f82e37

marker-fold-after.png

Row by row

Text below is the literal innerText read off each rendered row, not a transcription of the image.

what the user's turn actually is before after
a source turn for one image, then [Image #1] compare with the [Image #2] I mentioned earlier attachment chip + compare with the I mentioned earlier — the second marker is deleted from what the user sees attachment chip + compare with the [Image #2] I mentioned earlier — one image, so exactly one marker is absorbed and the surplus stays as the text it is
[Image #1] hello, typed with no image attached hello — the ticket's defect [Image #1] hello
[Image #1]: /Users/brennan/Desktop/dashboard.png : /Users/brennan/Desktop/dashboard.png — marker eaten, orphan colon left behind [Image #1]: /Users/brennan/Desktop/dashboard.png
[dashboard]: /Users/brennan/Desktop/dashboard.png completely empty bubble (visible in the before frame as the bare grey pill above Noted that one too.) — the Markdown parser reads the line as a link reference definition, which renders to nothing, so the user's message vanishes [dashboard]: /Users/brennan/Desktop/dashboard.png

The fourth row is what remark-disable-definitions.ts and the disableLinkDefinitions prop on NativeChatMessageList fix. Note that it is a pre-existing swallow — base loses that message too — but it becomes more reachable once markers stop being stripped, because [Image #1]: /path is itself definition-shaped.

Why the definition-disable plugin is load-bearing

Same after instance, same transcript, single variable: disableLinkDefinitions removed from the user branch of NativeChatMessageList and reloaded through renderer HMR. Both definition-shaped turns collapse to empty bubbles — including the exact [Image #1]: /path shape from the ticket, which the previous frame renders fine. Restoring the prop brought both rows back, so the prop is the cause of the difference and nothing else.

definition-swallow-head-without-plugin.png


What this does not cover

Stated plainly, because the gaps matter more than the frames.

  • The optimistic-echo / glue / content-key reconciliation work has no rendered proof here, including the fix where one transcript row was retiring two pending sends and permanently deleting the user's photo. That path needs a live agent session plus a real composer send carrying an image attachment. I could not reach it without doing something I am not willing to do on this machine: the composer's only attachment entry points are (a) the paste path, which reads the image off the system clipboard via window.api.ui.saveClipboardImageAsTempFile — reaching it means overwriting whatever the user currently has on their clipboard; (b) window.api.shell.pickAttachment, a native file dialog; and (c) window.api.ui.onFileDrop, a native OS drag. (b) and (c) are OS-level automation, which is barred for Orca UI validation. So: not reached, deliberately, and not partially claimed — a text-only send would exercise the echo plumbing but not the image budget, which is the part that was losing photos, so I did not capture one and pass it off as coverage. That work rests on its unit tests.
  • Mobile has no desktop-CDP surface at all. None of the mobile changes in this PR are covered by anything above.
  • The window-head case (a marker turn whose [Image: source: …] anchor was trimmed out of the read window, which must still strip) is not in these frames; it was captured in an earlier round on the earlier commits and the fixture here is deliberately short enough that no trimming occurs.
  • Frames are from the branch, not from a merged tree.

One thing seen and ruled out: both instances threw an unrelated React render error in sidebar dialog (the workspace list) when the synthetic agent-status entry was injected. It appeared identically on be07b43a2b and 13b9f82e37, so it is an artifact of the injection rig, not a regression from this PR. It was dismissed before the frames above were taken and is outside the cropped region.

`nativeChatUserMessageImageEvidenceCount` took `max(imageRefCount,
markerCount)`. That was equivalent while every marker was stripped from every
turn, so a folded image row could never keep one. This branch bounds the strip
to the size of the run it anchors to, so a row legitimately retains the markers
the user typed — and the max then reads them as extra photos.

Measured, through the real fold: a source turn for one image followed by
`[Image #1] look [Image #2] [Image #3]` folds to one image-ref plus two
surviving markers, and reports evidence for two photos while rendering one. Its
content key is then the key a two-photo send owns, so that send retires against
a row showing one image and `prunePendingSends` drops it from the bounded cache
with the user's photos in it. At the merge base the same input reports one.

Structure now wins wherever it exists; markers stay the fallback for a host that
echoes an image send as a bare `[Image #n]` with no source turn, which is the
case they were added for and which is untouched (it has no image-ref blocks).
Under-counting only costs a duplicate bubble; over-counting loses the photo —
the same trade `nativeChatUserTextRow` already makes on the glue arm, which was
guarded against exactly this and whose sibling call site this is.

Differential over 163,856 base-vs-head cases: cases where head loses a photo the
base kept drop from 408 to 288, with no new losses in the other direction and no
case of one transcript row retiring two sends.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Visual proof — the rendered surface, before and after

Earlier rounds reported no visual evidence, then partial evidence covering only the
window-trim fix. Here is the rendered surface for the three display behaviours this
PR actually changes, captured from two isolated dev instances driven over CDP.

  • Before — be07b43a2b (the merge base). Launch log records
    [orca-dev] Instance: vis14939-before @ be07b43a2b.
  • After — this branch.

Both instances read the same synthetic transcript through the normal transcript
reader, so the only variable is the branch.

Before

before

After

after

What the pair shows, row by row

the turn before after
one real attachment, prose mentioning a second marker compare with the I mentioned earlier — the user's [Image #2] deleted compare with the [Image #2] I mentioned earlier
a marker typed with nothing attached hello — the [Image #1] deleted [Image #1] hello
a turn that is only [Image #1]: /path.png : /Users/brennan/Desktop/dashboard.png — label gone, line mangled renders as typed
a turn that is only [dashboard]: /path.png an empty bubble — CommonMark reads it as a link reference definition, which produces no output, so the message vanishes entirely renders as typed

The empty bubble in the "before" frame is the one worth looking at twice: it is a
message the user sent, rendering as nothing at all. That is what the parser-level
construct disable fixes, and it is the failure the earlier source-text escape did not
fully cover.

The attachment itself still folds correctly in both frames — the dashboard.png chip
is present on the image turn either way. Anchoring changes which markers are treated
as placeholders, not whether real images render.

What is NOT in these frames, stated plainly

  • The optimistic-echo / glue / content-key reconciliation work has no rendered
    surface reachable this way.
    Exercising it needs a live agent session plus a real
    composer send with an attachment, so that a queued bubble exists to reconcile
    against a landed turn. The synthetic-transcript rig binds a read-only transcript and
    cannot produce a pending send. That work rests on unit tests and on base-vs-head
    differentials, not on these screenshots.
  • Mobile has no desktop-CDP surface, so none of the mobile arm is covered here.
  • The frames were captured off the branch, not off a merged tree.
  • The "after" instance was built at 13b9f82e37. The commit added since
    (5011bf0b8d) changes reconciliation counting only and touches no render path, so
    these frames remain representative of the branch's rendering.

Images are attached to this repository; nothing was uploaded to a third-party host.

…wards

`useMobileNativeChatMessageSend` gained `captureSendOrigin(text, images)` on this
branch, and nothing asserted the second argument. The assertion that used to pin
the call was narrowed to the first argument when the parameter was added, and
every other test in the area calls `captureSendOrigin` directly from the drafts
hook, so the wiring between the two was uncovered.

Measured: dropping `images` from the call turns exactly ONE test red out of 1240
in the mobile session suite — this one. Without it an image send captures
`imageCount: 0`, so its origin keys as a markerless text send, an `[Image #n]`
caption stays literal, and the optimistic echo can never match its own landed
turn — a photo bubble queued forever.
…led to

Mobile's `MAX_MESSAGES` and the runtime RPC host's
`MOBILE_NATIVE_CHAT_MAX_WINDOW` are both 2000, and the load-earlier affordance
only terminates because they are equal. Nothing said so at either site, so
raising one alone would silently restore a "Load earlier" button that re-reads
the whole window on every press and never loads anything — the failure this
branch already fixed once from the other direction.

Comment only; no behaviour change.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review rounds 6–7 — a P1 found and fixed, and every filed follow-up dispositioned

The previous summary reported round 4 as CLEAN, but a commit landed within a minute of
it, so the top of the branch had never been reviewed. Picking the loop back up found a
real defect — which is the expected result here, since a majority of everything found on
this PR was introduced by an earlier fix on it.

The P1: markers outnumbering the photos on the same row

nativeChatUserMessageImageEvidenceCount returned max(imageRefCount, markerCount).
That was equivalent for as long as every marker was stripped from every turn — but
this branch bounds the strip to the size of the run it anchors to, so a row can now
legitimately keep markers the user typed, and the max read them as extra photos.

A one-image run whose prompt is [Image #1] look [Image #2] [Image #3] folds to one
image-ref plus two surviving markers, and reported evidence for two photos while
rendering one. That is exactly the key a two-photo send owns, so the row retired it
and prunePendingSends dropped it from the bounded cache with the user's photos in it.

Three-way, same transcript, same pendings:

prunePendingSends
merge base ['p-two'] — correct
branch before the fix ['p-one'] — the two-photo send deleted
branch after the fix ['p-two'] — base outcome restored

Structure now wins wherever it exists; markers remain the fallback only for a row
carrying no image-ref at all. Under-counting costs a duplicate bubble; over-counting
loses the photo — the same trade the glue arm already makes, and this was its sibling
call site, which is where every previous defect on this PR turned up.

Evidence. Reverting only the production line turns exactly the two new tests RED
(123 others still pass, no compile failures). A 163,856-case base-vs-head differential
moves cases where the branch loses a photo the base kept from 408 to 288, with none
in the other direction and no case of one row retiring two sends. An independent
40,000-case fuzz moves head-only photo loss from 2 to 1, with zero cases where the
fix loses a photo the pre-fix build kept; the survivor is a marker-only row the fix
cannot touch, where the base loses the same photo.

The fix also turns out to be strictly better than the base on the inline-image path some
agents use (an image block and the typed text in one content array): there the base and
the pre-fix branch both retired a two-photo send against a one-photo row, and the fix
keeps it.

A production line this branch added was covered by nothing

useMobileNativeChatMessageSend gained captureSendOrigin(text, images), and the
assertion that used to pin the call had been narrowed to the first argument. Dropping the
images argument turned one test red out of 1,240 — the one added here. Without it an
image send captures imageCount: 0, keys as a markerless text send, and its photo bubble
queues forever.

Correction to the record: the marker fallback is not speculative

It was suggested during review that no agent actually echoes an image send as a bare
[Image #n] with no source turn, which would have made the fallback dead code. Measured
against the decoder, it is real and it is the common path: an attachment stored as a
base64 source has neither a url nor a path, so the image block decodes to nothing and the
turn is left as bare marker text with zero image-ref blocks. A url-bearing or path-bearing
source still yields the block — the decoder is the discriminator, not the agent. This is
also why the fix is shaped the way it is: fall back to markers only where structure is
genuinely absent, which is precisely that case.

The filed follow-ups, each dispositioned

# item disposition
1 Mobile's unfiled instance of the glue-blindness defect Follow-up. Needs a captioned photo send plus an adjacent text send whose echo is one glued row. Both arms behave as they did before this branch; the cost is a duplicate bubble, not a loss.
2 Desktop has no glue slide, so one unretirable send disables glue retirement for the session Follow-up, pre-existing. Identical on the merge base. Porting mobile's slide budget is a behavioural change to the most incident-dense code here and is unrelated to this ticket.
3 Desktop glue has no per-send boundary, so a row can retire a send issued after it Follow-up, pre-existing. The photo-carrying variant is now blocked by the glue arm's image-count agreement rule; the text-only variant costs a prematurely hidden bubble.
4 A bound preview replaces a host image's url instead of appending beside it Follow-up, pre-existing, now pinned by a test so a change of behaviour is visible.
5 Mobile's MAX_MESSAGES cap silently coupled to the host's window clamp Fixed here (comment only). Both are 2000 and the load-earlier affordance only terminates because they are equal; neither site said so. Raising one alone would have restored the never-loading button this branch already fixed from the other direction.
6 A photo bubble stranded as the price of the one-row-one-send guard Accepted trade, unchanged. Confirmed during this round to be permanent rather than transient, in 977/40,000 fuzz cases. The photo is preserved; the alternative re-opens deletion. Recorded rather than repaired because the minimal-looking repair is not slice-invariant.
7 The subscribe hop does not type-guard a non-boolean paging answer Follow-up. The fold gates on === true, so a non-boolean reads as absent and cannot reach it; this is affordance-and-contract only.

Residuals recorded, not fixed

  • Old-host gap. The window-head rule only fires when the host reports its paging
    answer. Against a host predating that field, a read that cuts inside a multi-source run
    can leave surviving markers beside a real image-ref. Measured: for the send that
    produced the row the effect is a delay — it stays queued under both builds and
    retires correctly once the rest of the run pages in. For a colliding send with the same
    caption the branch trades a lost two-photo send for a lost one-photo send, and the
    merge base loses both, so it is no worse than the baseline. Closes on its own for
    any host carrying the field, and for old hosts as soon as one more page loads.
  • Perf. prunePendingSends is ~1.8x its previous constant, which is 0.19 ms/call at
    the realistic depth (default window, one send in flight) on a transcript-change path —
    not a keystroke path; both call sites are keyed on transcript identity and the composer
    draft is not view state. An empty queue early-returns. It only becomes material if a
    stranded send keeps the queue permanently non-empty at a heavily paged window.

What is still not established

Whether the window-head strip is ever wrong in practice — it is a documented trade that
self-corrects on load-earlier, and its frequency is not measurable from here. And whether
the stranded-send case can fill the whole eight-slot queue, which is what the perf worst
case assumes; one permanent strand is proven, a full queue is not.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Readiness review — closing pass at 575ac845ea

Run against the full diff, not just this session's commits.

Verdict: releasable. No P0. No P1 outstanding — the one P1 found this session
(a row vouching for more photos than it renders, which deleted a two-photo send from
the cache) is fixed and verified three ways. Two P2/P3 residuals are recorded below with
their trade stated.

01 — SSH / remote / network

No wire change. hasMoreReported is synthesized client-side at every adapter and
never crosses the remote boundary: it appears in the local IPC handler, the preload type,
the runtime transport adapter and the web preload, and zero times in
src/main/runtime/rpc/methods/native-chat.ts, which still sends only the pre-existing
hasMore. Verified by grep over src/ and mobile/src/.

Both mixed-version directions checked:

  • Old host → new client. The host omits the field; parseRuntimeNativeChatReadSessionResult
    keeps hasMore only under typeof === 'boolean', so earlierHistoryConfirmed stays
    false, the window-head rule never fires, and markers are preserved. That is the safe
    direction — the ticket's fix stays on and only the trimmed-head strip is skipped.
  • New host → old client. The old client never sees a new field, because none is sent.
    It reads hasMore exactly as before.

Neither direction can strip an [Image #n] the user typed: earlierHistoryConfirmed
only becomes true from a host-answered boolean via === true, and the host computes that
exactly by over-reading one turn past the limit. No new opcode, so no capability
negotiation is required.

02 — Crash / cast / retry / unbounded growth

No new unbounded growth: the pending queue is LRU-bounded at 8 and the transcript window
is clamped by the host. Every new read of an untrusted field is type-guarded
(typeof record.hasMore === 'boolean', reportedHasMore()), which matters because the
remote read casts host frames without validating per-field types. The fold's window-head
comparison deliberately refuses to match an undefined id, so a row arriving without one
cannot satisfy the rule. No new retry loops, timers, listeners or awaits.

03 — Security / supply chain

No dependencies added, no lockfile change, no shell or eval, no new network sink, no
secrets. The one parser change reduces surface — it disables two CommonMark constructs
for user-typed content — and rehype-sanitize still runs on the same pipeline.

04 — Performance

Measured, not asserted. The fold is 2.4–4x faster than the merge base at both the
desktop (300) and mobile (2000) window caps, stable across repeated runs, because
marker-free rows take one findIndex instead of a regex replace per block, and the input
reference is returned when nothing folds. The reconciliation pass costs a constant factor
more (0.19 ms/call at the realistic depth of one in-flight send) on a transcript-change
path, not a keystroke path — both call sites are keyed on transcript identity and the
composer draft is not view state. An empty queue early-returns in ~0.00002 ms.

05 — Functional correctness

Seven review rounds. The user-visible behaviour is confirmed rendered, not just
unit-tested — see the before/after frames posted above, which show the literal marker
surviving, the surplus marker surviving, and a message that previously rendered as a
completely empty bubble.

06 — Backward compatibility & data loss

The central risk on this PR, and the reason for the differentials. No persisted state,
schema, migration or serialized contract is touched — the pending cache is in-memory and
LRU-bounded. Photo loss is measured rather than argued: a 163,856-case differential moves
cases where the branch loses a photo the base kept from 408 to 288, with none in the
other direction; an independent 40,000-case fuzz moves head-only loss from 2 to 1, with
zero cases where the fix loses a photo the previous build kept.

Mobile back-compat holds in the same direction as the wire analysis: a host without the
paging field leaves the fold off, which preserves user text.

07 — Cross-platform & remoting

No platform APIs, paths, shortcuts, process handling, PTYs, watchers or native modules.
This is transcript normalization plus renderer-local bookkeeping, so macOS / Linux /
Windows and local / SSH / relay behave identically. Not manually exercised on Linux,
Windows or SSH, and there is no code path where they could differ.

Residuals (accepted, not fixed)

  • P2 — a photo bubble can strand permanently. The one-row-one-send guard forces a row
    to the literal reading when any no-image send is queued, so a marker echo for a
    different photo send never retires. Confirmed permanent, in 977/40,000 fuzz cases.
    The photo is preserved; the alternative re-opens deletion, and the minimal-looking
    repair is not slice-invariant.
  • P3 — the reconciliation constant factor described under 04. Only becomes material
    if a stranded send keeps the queue non-empty at a heavily paged window.
  • Old-host gap. Against a host predating the paging field, a read cutting inside a
    multi-source run leaves surviving markers beside a real image-ref. For the send that
    produced the row this only delays retirement — it recovers once the run completes,
    verified by exhaustion. For a colliding same-caption send the branch trades a lost
    two-photo send for a lost one-photo send, and the merge base loses both.

Not established

Whether the window-head strip is ever wrong in practice — a documented trade that
self-corrects on load-earlier, whose frequency is not measurable from here. And whether
the stranded-send case can fill the whole eight-slot queue, which is what the perf worst
case assumes; one permanent strand is proven, a full queue is not.

@brennanb2025

brennanb2025 commented Aug 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Approach pass, re-derived

An approach pass ran on this PR earlier. This one was derived independently rather than by
re-reading that report, and it reaches one conclusion the earlier pass did not.

1. Rendering prose the user typed as Markdown — diverges, but justified

The most conservative design available is stricter than we are: route assistant turns
through a Markdown renderer and render the user turn as raw text with no Markdown parsing
at all. Two different components, chosen by role.

That is the same principle this PR is built on — prose a human typed must not be silently
transformed by Markdown semantics — taken further than we take it. We keep Markdown
affordances on user turns and disable only the two constructs that delete content outright.
Given that Orca already ships Markdown rendering for user turns and people rely on it,
disabling the destructive constructs is the minimal correct repair, and switching user
turns to literal text would be a product change well outside this ticket.

Worth recording as a possible future simplification: if we ever want to stop reasoning
about which constructs are safe on user prose, rendering user turns literally is the
answer, and it removes the whole class rather than the two known members.

2. Structure versus a count scraped out of text — no external model to borrow; strong internal precedent

There is no established pattern for reconciling a structural count with a count parsed out
of message text, because the situation is peculiar to reading an agent's transcript rather
than receiving structured events. Reporting that as "nothing to borrow" rather than
manufacturing a match.

The precedent that does exist is Orca's own, in two places, and both already chose
structure-first:

  • the glue arm counts image-ref blocks only, with a comment stating the reason —
    over-counting loses the photo, under-counting only costs a duplicate bubble;
  • the mobile preview-binding path tests structure first and consults markers only in a
    separate branch guarded on the turn having no text.

The content-key arm was the one place taking the larger of the two, and it is the one place
this caused data loss. The change makes the three agree. That is convergence on an existing
internal rule, not a new invention.

3. Matching an optimistic bubble to its echo by content — forced by the medium, not a shortcut

This is the conclusion the earlier pass did not reach, and it matters because content
matching is the root of the most incident-prone code in this area. The obvious criticism is
that we should thread a client-supplied idempotency key through and match on that instead.

Checked against the actual record formats the agent CLIs write, and it is not available.
The user-turn record carries role, content, and an optional phase; the id that exists on
the sibling variant is an optional provider-assigned response id, not a client token, and
it is absent on the shape a user turn takes. There is no field a client could populate and
read back.

So Orca is not choosing content matching over a cleaner correlation-id design — the
correlation id has nowhere to live without changing an agent CLI's own on-disk format. That
reframes the whole area: the content-key machinery is the cost of reading a transcript
written by a process that does not know we exist, and the right engineering response is to
make its failure modes safe (which is what the one-row-one-send guard and the structure-first
count do), not to keep looking for a key that is not there.

Verdict

Nothing hacky. The Markdown fix is a narrower version of a sound pattern; the
evidence-count fix converges on a rule this codebase had already written down twice; and the
reconciliation architecture is forced by the medium rather than chosen. The one item worth
carrying forward is the note in §1, as a simplification rather than a defect.

… is coupled to"

This reverts 575ac84. The coupling it documented does not exist, and the
comment asserted a mechanism the code does not have.

The claim was that load-earlier terminates only because `MAX_MESSAGES` (2000)
equals the runtime host's `MOBILE_NATIVE_CHAT_MAX_WINDOW` (2000), so raising the
client cap alone would leave a button that re-reads the same capped tail forever.
Measured against the real hook, both paths refute it:

- the cursor path asks for a PAGE (`pageLimit`, 60) with a `beforeOffset` on every
  read, never the whole window, so the host's window clamp cannot bite at all;
- the growing-tail path terminates through its own count inference — driven
  against a host clamped to 100 while the client cap stayed 2000, the affordance
  went false after two presses (limits 100 then 160), where the coupling claim
  predicts it stays true forever. An unclamped control arm kept it true for all
  eight presses, so that arm was observing the clamp and not an unrelated stop.

Termination comes from `nextLimit < MAX_MESSAGES` and the `pageLimit <= 0` early
return, neither of which involves the host. There is no invariant here worth
naming, so the comment goes rather than gets reworded.

The claim was inherited from an earlier residual note and committed without being
checked, which is the error the review loop exists to catch.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Correction: the MAX_MESSAGES coupling I documented does not exist

In the follow-up table above I listed item 5 — "mobile's MAX_MESSAGES cap silently
coupled to the host's window clamp" — as fixed here. That was wrong, and the commit
that "fixed" it has been reverted (4d5711c165).

The claim, inherited from an earlier residual note and repeated by me without checking,
was that load-earlier terminates only because MAX_MESSAGES (2000) equals the runtime
host's MOBILE_NATIVE_CHAT_MAX_WINDOW (2000) — so raising the client cap alone would
leave a "Load earlier" button re-reading the same capped tail forever. Driven against the
real hook, both paths refute it:

  • The cursor path never asks for the window. Every read requests pageLimit (60) with
    a beforeOffset, so the host's window clamp cannot bite and "the same capped tail"
    cannot occur. Measured: two presses, both limit: 60, both carrying a cursor.
  • The growing-tail path terminates on its own count inference. With the host clamped to
    100 while the client cap stayed 2000 — the exact configuration the comment warns about —
    the affordance went false after two presses (limits 100 then 160). The coupling claim
    predicts it stays true indefinitely.
  • Control arm: the same rig against an unclamped host kept it true for all eight presses
    (limits 100 through 520), so the terminating arm was observing the clamp rather than some
    unrelated stop.

Termination comes from nextLimit < MAX_MESSAGES and the pageLimit <= 0 early return,
neither of which involves the host. There is no invariant here worth naming, so the comment
was removed rather than reworded.

So item 5 belongs in the same column as the rest: not a defect, and nothing to fix. The
only real change is that a note claiming an undocumented hazard has been retired, because
the hazard is not there.

Worth recording as a process point, since it is the same failure this PR keeps producing:
the false claim reached a commit because it was carried forward from a previous summary
instead of being re-derived. Every other item in that table was measured; this one was
inherited. It was caught by the next review round, which is the argument for running the
loop past the point where the diff looks settled.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review loop — closed at round 10, clean with no changes

An earlier summary reported the loop closed at round 4. It had not: a commit landed
within a minute of that verdict, so the top of the branch had never been reviewed.
Picking it back up found a data-loss regression, which is the argument for running this
past the point where the diff looks settled.

round verdict what it found
6 NOT CLEAN P1 — surplus markers inflating the image budget, so a row rendering one photo retired a send carrying two and prunePendingSends dropped it from the cache
7 CLEAN, no changes attacked the P1 fix from four directions and could not break it
8 NOT CLEAN a comment added in round 7 asserted a mechanism the code does not have; the PR body's Testing section was stale
9 probes clean, no verdict delivered asked the parser-exposure question nobody had; replaced rather than counted
10 CLEAN, no changes independently re-verified the one changed production line; found the multi-image echo ordinal is a strict improvement over base

The stopping rule is a round that comes back clean having changed nothing. Round 7 was
clean but a commit followed it; round 9 produced good probes but never delivered a verdict,
so it was replaced rather than counted. Round 10 is the first that both returned clean and
was followed by no change, and nothing has been pushed since.

What changed

Four commits, one production line between them:

  • 5011bf0b8d — the P1 fix: the evidence count now trusts structure and falls back to
    markers only where structure is absent.
  • 6612c0103d — a test for a production line this branch added that nothing covered.
    Dropping the argument turned one test red out of 1240: the one added for it.
  • 575ac845ea / 4d5711c165 — a comment, and its revert. See below.

The comment I got wrong

575ac845ea documented a coupling between mobile's MAX_MESSAGES and the host's window
clamp, claiming load-earlier terminates only because the two are equal. Round 8 drove the
real hook against a host clamped to 100 while the client cap stayed 2000 — the exact
configuration the comment warns about — and the affordance terminated after two presses.
A control arm against an unclamped host kept it alive for all eight. The cursor path never
asks for the window at all; it requests a 60-row page with a cursor, so the clamp cannot
bite. Termination comes from nextLimit < MAX_MESSAGES and the pageLimit <= 0 early
return, neither of which involves the host.

The claim was carried forward from an earlier summary rather than re-derived, and that is
how it reached a commit. Reverted, and the follow-up table above corrected.

The strongest property established

The fold never strips more than the base does. Verified twice by independent
implementations — 40,000 random transcripts against every window-head choice, and a
separate exhaustive sweep — with both runs confirming the two folds genuinely disagree
somewhere, so the property is not vacuously true. In plain terms: on no input can this
branch delete user text that the previous behaviour kept.

Alongside it: for every row that carries an image-ref, the fixed evidence count is exactly
base-equivalent, because the base fold stripped unconditionally and so every post-fold row
reaching that function had zero markers. The only behaviour this branch introduces is on
rows with no image-ref at all — a case the base could not reach. So the fix cannot regress
the base on any input; it can only remove the over-count the bounded strip newly created.

Security question raised by the Markdown change

Turning off the definition and gfmFootnoteDefinition constructs means bytes CommonMark
previously swallowed now reach the inline parser. That is a real exposure change and it
had not been examined. Tested with javascript: and vbscript: destinations, a raw tag as
a destination, a title carrying raw HTML, and a footnote autolink — each against a control
arm proving non-definition-shaped input is unaffected. The bytes are exposed to the parser
and neutralised by the existing sanitizer; an ordinary https: target renders as the link
the user typed.

One thing round 10 found that is worth keeping

The multi-image echo ordinal is not just safe, it is a fix. appendMobileNativeChatPending
now gates the expected image-echo ordinal on the pending's own image count, and the preview
binder builds a per-entry size-filtered candidate list that the ordinal indexes into. Driven
both orders, each side through its own modules: on the base, sending a one-photo message and
then a two-photo message left the two-photo send's ordinal pointing past its one-element
candidate list, so its preview never bound at all. On this branch both bind. The
decreasing-size order passes on both, so the probe discriminates rather than failing blanket.

Evidence summary

  • Desktop 92 files / 1010 tests; mobile 134 files / 1240; every file mentioning native
    chat by content rather than path, 153 files / 2841; both typechecks exit 0; lint
    clean with the gate proved live by planting a violation and watching it get reported.
  • Non-vacuity established three ways for the P1 fix — source mutation, module mocking, and
    mutant kill-set scoring, which showed only one of its three new cases actually
    discriminates the fix from its parent.
  • Base-vs-head differentials, each side through its own pipeline: 163,856 cases
    (photo loss 408 → 288) and an independent 40,000-case fuzz (2 → 1, none the other way).
  • Visual before/after posted above.

Note on the readiness pass above

It is headed 575ac845ea, which is no longer the tip. The only change since is the revert
of a comment, so every finding in it still holds at 4d5711c165 — and the one item it
would otherwise have overstated (the MAX_MESSAGES coupling) is the thing that was
reverted.

Residual risks, unchanged

The bounded duplicate-bubble strand; the reconciliation constant factor; the old-host gap
where the fix delays rather than loses. Two mobile landing paths disagree on a row with one
image-ref and surplus markers — nobody found a route to that row, and if one exists the
symptom is a transient false "delivery unknown" that self-heals, with no data loss.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing this stale implementation and carrying forward the remaining fix: typed image-marker text must stay visible when it is not an attachment placeholder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant