Skip to content

fix(native-chat): enforce each pending send's own boundary in glue matching (STA-4477) - #14935

Merged
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/sta-4477-desktop-glue-send-boundary
Aug 17, 2026
Merged

brennanb2025 merged 3 commits into
mainfrom
brennanb2025/sta-4477-desktop-glue-send-boundary

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Aug 16, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​87 $\color{#cf222e}{\Huge{\mathbf{−}}}$​7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​80
Prod 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​209 $\color{#cf222e}{\Huge{\mathbf{−}}}$​156 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​53

ELI5

When you fire off two chat prompts in a row, the agent's terminal sometimes swallows them onto one line, so both show up in the transcript as a single user turn. Native chat knows how to recognize that and clear the two "queued" bubbles. But it was checking that merged row against only the oldest waiting prompt — so a prompt you typed after that row already landed could also be marked "already delivered" and quietly thrown away. This makes every waiting prompt prove the row came after it, individually.

What Changed

Desktop / renderer only. mobile/ is untouched.

  1. Every pending send carries its own transcript boundary into glue matching. gluedCandidateRows now tags each candidate row with the set of pending indices that row actually landed after, instead of filtering the whole candidate list by open[0]'s boundary and then matching the entire open queue against it.

  2. A send the row predates ends the run instead of being skipped. In selectPendingIndicesRepresentedByUserRows, a candidate that the row cannot represent breaks out of the open run rather than being filtered out of it. Glue consumes a leading, adjacent run — adjacency is the whole reason a merged row is attributable at all, so skipping over a non-representable send would let a row claim two prompts that were never adjacent.

  3. Exact matches are untouched. gluedCount < 2 still returns to the occurrence/content-key path, so repeated prompts and ordinary single landings keep their existing semantics.

  4. Forced file split, pure move. native-chat-pending.ts was at 299 of its 300 effective-line budget (max-lines, skipBlankLines + skipComments) on main. One added line breaks the build, and this repo never bumps or disables max-lines. The slash-command marker cache — a genuinely separate rule that never participated in pending pruning — moves verbatim into native-chat-command-marker.ts. Verified byte-identical to the code it replaced; only import sites changed.

Why

prunePendingSends and pendingSendsAsMessages both derived their glue candidates from a single anchor:

const oldest = open[0]
return messagesAfterPendingBoundary(messages, anchor)   // ← one boundary for the whole queue

That is safe only while the queue is a single burst. As soon as one prompt is queued after a glued row has landed, that older row is still in the candidate list for the newer prompt, and if the row's text happens to spell the concatenation of the queue ("fix the bug" vs "fix the" + "bug"), the newer prompt is pruned as already-delivered. The bubble vanishes and no transcript turn ever appears for it — the prompt is simply lost.

A fuzzy match must never reach further back than an exact one. The exact paths already call messagesAfterPendingBoundary(messages, entry) per entry; the glue path was the only match path without a per-send boundary.

Linked Issue

Refs STA-4477. Original PR #14663.

Visual Proof

Not applicable — pure non-visual matching logic behind existing UI, with no rendered change in any passing case. The user-visible effect of the bug is the absence of a bubble, which the deterministic tests below assert directly on both the pruning and the rendering entry point.

Testing

pnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/native-chat
pnpm exec tsc --noEmit -p config/tsconfig.tc.web.json
PATH=/opt/homebrew/opt/node@24/bin:$PATH node config/scripts/check-changed-code-quality.mjs -- origin/main
pnpm exec oxfmt --check src/renderer/src/components/native-chat/*.ts src/renderer/src/components/native-chat/NativeChatView.tsx

Results at this HEAD, rebased onto bf6dc6fcba:

  • Native-chat suites: 73 files / 768 tests passed.
  • tsc --noEmit (web project): clean. tsc -b deliberately not used — its emit poisons vitest.
  • Changed-code quality, type-aware quality, React Doctor: 0 new findings across 6 changed files.
  • Oxfmt: clean on all 5 changed source files.

Red-first / non-vacuity. Reverting the production files wholesale is not a clean measurement here, because the fix renames the matcher's API and 2 of 3 test files then fail to load rather than fail an assertion. So the behavioural revert was done surgically instead — representablePendingIndices set to every index, which is exactly the pre-fix "one anchor for the whole queue" semantics with the new signature:

× keeps a queued send whose own boundary is newer than the glued row (STA-4477)
  AssertionError: expected [] to deeply equal [ { id: 'p1', …(4) }, …(1) ]
  Test Files  1 failed (1)
       Tests  1 failed | 61 passed (62)

That is the P1 verbatim: both pending sends pruned ([]) where only the older one had actually landed. The companion test still retires the leading run the glued row did land after stays green under the same mutation, so the fix is pinned from both sides and is not simply disabling glue.

STA-4477 was re-confirmed present on current origin/main before fixing — gluedCandidateMessages there still filters by open[0] alone.

Review

  • Security / supply chain — no dependencies, IO, network, secrets, shell, or persisted state.
  • Backward compatibility / data loss — this removes a data-loss path; it never widens retirement. The added per-send bound can only make the matcher retire fewer entries, never more, so the failure direction is a bubble lingering (recoverable, visible) rather than a prompt disappearing (silent, unrecoverable).
  • Remote wire — none. NativeChatPendingSend is renderer-local state; nothing here is an RPC param, a stream frame, or content a host publishes. Checked against docs/reference/remote-wire-compatibility.md rules 1–3.
  • Cross-platform — pure string/array logic; no platform, path, or shell branch. macOS/Linux/Windows identical.
  • Performance — one extra Set per open pending entry per pass, bounded by PENDING_SEND_LIMIT = 8; all allocations invocation-local.
  • Scope — 6 files, all under src/renderer/src/components/native-chat/. The file split is forced by the 300-line cap, not elective.

Checklist

  • Root cause identified and fixed at the seam that lacked the bound
  • Red-first reproduction quoted, with a mutation that isolates the behaviour from the API rename
  • Over-correction pinned by a companion test
  • File split proven a verbatim move, forced by max-lines (never bumped, never disabled)
  • Wire, cross-platform, and data-loss direction reviewed

AI Disclosure

Authored and reviewed with AI assistance.

Author

  • X / Twitter: @BrennanKB5

…tching

Glue matching filtered candidate rows against the OLDEST still-open send and
then matched the entire open queue against them. A prompt queued after a glued
row landed could therefore be judged "already delivered" by that older row and
pruned — the queued prompt disappeared with no bubble and no transcript turn.

Each send now carries its own transcript boundary into the match:
`gluedCandidateRows` tags every candidate row with the set of pending indices
it actually landed after, and the matcher stops a run at the first send the row
predates rather than skipping over it (adjacency is what makes a row glue).
Exact single matches still belong to the occurrence path, unchanged.

`native-chat-pending.ts` sat at 299 of its 300 effective-line budget, so the
slash-command marker cache — a separate rule that never took part in pending
pruning — moves verbatim to `native-chat-command-marker.ts`. Pure move: no
behavior change, imports only. (max-lines is never bumped or disabled.)

Refs STA-4477. Original PR #14663.
@coderabbitai

coderabbitai Bot commented Aug 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The change moves command-marker storage and processing into a dedicated module. Transcript matching now returns message IDs with normalized text. Pending-send glue matching tracks representable pending indices and respects send boundaries. Pending pruning and optimistic rendering use the new row-based APIs. Tests cover boundary ordering, adjacency, partial retirement, and visibility. The benchmark now exercises and asserts the row-based matching flow.

Merge Risk: 🟡 Moderate · up to e57a3

The change fixes per-send transcript boundaries, but an image-only pending message can still be skipped when matching a merged row, potentially causing surrounding text prompts to be incorrectly retired and lost. Merge should wait for this bounded correctness issue to be fixed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 39.13% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the native-chat glue-matching fix and the per-send boundary change.
Description check ✅ Passed The description covers the change, rationale, linked issue, visual proof, testing, review, checklist, and authorship; only non-critical template sections are omitted.
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.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review: CLEAN — no blocking defects

Re-verified against head 9fef6a6 (my checkout matches the PR head exactly). CI: 45 pass, 1 skipped, 0 failures.

Read this first: #14935 and #14980 are complementary, not alternatives

This PR reconciles an already-merged transcript row downstream. The reason a merged row exists at all is that Orca manufactures it. Verified in the code at this head:

  • src/shared/agent-prompt-injection.ts:5 — the submit key is '\r', written as its own write, separate from the body.
  • src/main/runtime/orca-runtime.ts:17700-17718 (writeTerminalAgentPrompt) — writes the body chunks, awaits, then writes AGENT_PROMPT_SUBMIT. The wait is a render gate when the agent supports settlement, otherwise a flat setTimeout(AGENT_PROMPT_SUBMIT_DELAY_MS) — 500 ms, or 1500 ms on Windows (agent-prompt-injection.ts:7-8).
  • src/main/runtime/rpc/methods/terminal.ts:1356-1363 gates that settled path on params.client?.type === 'desktop'. Everything else falls through to writeTerminalAction, which has its own body→Enter gap at orca-runtime.ts:17605.

Two overlapping sends sit in their gap concurrently and interleave, landing as one agent input line. #14980 (draft) adds the per-PTY submit queue that closes that.

This PR is still worth landing on its own. Not just "defense in depth" — there is a concrete hole. A per-PTY send queue already exists for the desktop composer (native-chat-pty-send-queue.ts, doc'd as "Serialized per PTY so rapid sends cannot glue before Enter"), and it is renderer-local. Senders that bypass it entirely on the same PTY include mobile and four orchestration entry points (coordinator-task-dispatch.ts:131, rpc/methods/orchestration.ts:1533, orchestration-workers.ts:234, orchestration-federation.ts:228). So a merged row can still be produced by cross-client concurrency, and this PR is the only thing that keeps a queued prompt from silently vanishing when it is.

Scope note, which explains why the desktop and mobile halves are separate PRs: this change is desktop-renderer-only. Mobile does not share native-chat-pending.ts — it has its own mobile/src/session/mobile-native-chat-pending-echo.ts, and #14980 carries the mobile side.

Two things I turned up that belong to #14980, not here, but should not get lost:

  1. The Windows widening to 1500 ms applies only to the settled/agent-prompt path. The fallback path mobile takes uses a hardcoded literal 500 (orca-runtime.ts:17605) rather than the shared constant, so it silently drifts from it.
  2. No test asserts per-PTY serialization of the runtime write/wait/write sequence. rpc/terminal-agent-prompt-send.test.ts:19,53 covers the desktop gate only.

Prior-art conformance: (a) CONFORMS

Head-of-queue-only run consumption is the established discipline for this problem, and this fix restores it. Before, the matcher filtered candidate rows against the oldest still-open send and then matched the whole open queue against them, letting a run skip over a send it should have stopped at. Now it breaks at the first send the row predates instead of continueing past it (native-chat-pending-occurrence.ts:191-193) — adjacency is what makes a row glue, so a non-adjacent send ends the run. No (c) divergence anywhere in this PR.

Non-vacuity — what is actually test-proven

Three surgical mutations against native-chat-pending.test.ts (62 tests, green at head):

Mutation Result
A. Disable the per-send boundary guard (native-chat-pending-occurrence.ts:191) — i.e. restore pre-fix behavior 1 RED, exactly the new keeps a queued send whose own boundary is newer than the glued row (STA-4477) test
B. Break the guard at only the pendingSendsAsMessages call site, leaving prunePendingSends intact 1 RED, on the toHaveLength(2) assertion — the two call sites are independently guarded, not covered by one shared assertion
C. Over-apply the guard (no row can represent any send) 10 RED, incl. the second new test — the over-correction direction is fenced too

The regression test is a real regression test in both directions, and the fix is proven at both consumers.

Lint gate proven live, not inferred. The commit moves the slash-command marker cache out to native-chat-command-marker.ts to stay under max-lines. I did not take the silent clean run as evidence — I planted 70 code lines and confirmed oxlint fires: error eslint(max-lines): File has too many lines (307). help: Maximum allowed is 300. The commit message's numbers check out exactly: pre-PR native-chat-pending.ts was 299 effective lines against a 300 budget; post-split it is 237, with 77 in the new file. Unsplit-with-the-fix would be ~313 — over budget. The split was necessary, and max-lines was neither bumped nor disabled.

Move is verbatim. I diffed the moved block against the new file body: byte-identical. All six moved symbols have their consumers updated (NativeChatView.tsx, both test files) — no stale imports, no dead copy left behind.

What is NOT covered — please weigh these

  1. This PR never received an independent same-model review round. Zero reviews, zero inline comments, one auto-generated bot comment that hit its own rate limit. Everything above is my verification, not a second opinion.
  2. No screenshots, and this is a rendered surface. pendingSendsAsMessages feeds the rendered message list directly (NativeChatView.tsx:292); the user-visible symptom is a queued prompt disappearing with no bubble and no transcript turn. That symptom is proven at the unit level only — nobody has watched the bubble survive in a running app.
  3. The root-cause section is read from the code, not reproduced. I did not drive two overlapping sends through a live PTY to observe a glued row.

Everything in the mutation table and the lint/line-count section is executed and test-proven. The root-cause citations are read directly from the files at this head. Items 1–3 are the honest gaps.

…er path

The `break` on a send the row cannot represent is the fix's central semantic
choice, and swapping it for `continue` was passing the whole suite: nothing
exercised a queue whose middle send is unrepresentable. Add that case.

The mixed-age case also asserted both call sites in one `it`, so a prune-path
failure masked the render-path assertion — and the render path is the one that
makes a queued bubble visually vanish. Split it.

Skip the per-send boundary scans when fewer than two sends are open: the glue
matcher already returns nothing there, so a lone queued echo was walking the
transcript twice per render for a discarded result.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Electron QA (desktop native-chat glue send boundary)

Isolated PR-head instance (Orca: sta4477-qa-14935, CDP 127.0.0.1:9347, renderer 127.0.0.1:5291, fresh ORCA_DEV_USER_DATA_PATH). First attach: window.api.repos.list() returned []. Driver was Playwright CDP only.

What was staged

Opened a native-chat Claude pane on a throwaway local project (experimental native chat on). Sent two real composer prompts so optimistic queued bubbles appeared:

  1. QA-4477-PENDING-1 please reply with only the word pong
  2. QA-4477-PENDING-2 continue

Then wrote one concatenated user row (no assistant turn) into the pane’s watched transcript:

QA-4477-PENDING-1 please reply with only the word pongQA-4477-PENDING-2 continue

Those two pending bubbles retired and the glued row rendered. After that row was already on screen, sent a third composer prompt:

QA-4477-PENDING-3 after glue

Then, on the same instance, edited gluedCandidateRows so every glued candidate row was representable by every pending index (oldest-only anchoring), let Vite HMR reload, screenshot that state, restored the file from HEAD, and resent C on the restored fix.

What was observed

  • Native chat empty state, composer, and queued pending bubbles render on this branch.
  • After the glued transcript row landed, the third queued bubble survived with the PR matcher.
  • After HMR to the oldest-only matcher, that third bubble vanished; only the glued row remained.
  • After restoring HEAD and resending C, the third bubble survived again.
  • Console: 0 errors. Playwright reported 4 warnings the whole session (tooltip controlled/uncontrolled flip, a dev overlay version notice, a terminal deadline warning). None were new after the sends or HMR. No recoverable-UI-error dialog.

AFTER (fix): glued row present, QA-4477-PENDING-3 after glue still queued

06-after-c-with-fix.png

BEFORE (oldest-only matcher via HMR): same glued row, third bubble gone

07-before-old-matcher.png

AFTER restored + resent C: third bubble back

09-after-resend-on-fix.png

Queued bubbles before the glued row:

03-pending-after-send.png

04-two-pending-bubbles.png

What was NOT reproduced

Live agent glue was not produced. A running agent never concatenated two fast TUI sends onto one input line. The glued user row was written into the watched transcript after two real composer sends. HMR of the pending module cleared the in-memory queue, so the restored-fix confirmation is a resend of C, not the same cache entry coming back. Not exercised: mobile, Windows/WSL, SSH/Linux, or a real assistant reply after glue.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review session: CLEAN to merge — one review round found a real test gap, fixed in ee94f1bc

Second review pass on this PR, this time with an independent review round, precedent grounding, and desktop QA. Head is now ee94f1bc39 (was 9fef6a69).

What changed since the last review

The prior pass signed this off as clean but named three honest gaps: no independent review round, no screenshots on a rendered surface, and a root cause read from code rather than reproduced. All three are now closed, and closing the first one found a real defect — not in the fix's behavior, but in what the tests actually pinned.

ee94f1bc — the adjacency break was completely uncovered. The break in selectPendingIndicesRepresentedByUserRows (native-chat-pending-occurrence.ts:192) is the fix's central semantic: a send the row predates ends the glue run rather than being skipped over, because adjacency is what makes it glue. Swapping that break for continue passed the entire suite — 62/62 green. Nothing exercised a queue whose middle send is unrepresentable. That case is now covered, and the same mutation is 1 RED at head.

Two other things in that commit:

  • A masked assertion. The mixed-age test asserted prunePendingSends and pendingSendsAsMessages in one it, so a prune-path failure short-circuited before the render-path assertion ever ran — and the render path is the one that makes a queued bubble visually vanish. Split into separate tests. Reverting the guard at only the render call site is now 1 RED, so the two call sites are provably independently guarded.
  • The early-out was on the wrong side of the work. selectPendingIndicesRepresentedByUserRows bails at pending.length < 2, but gluedCandidateRows(...) was evaluated as an argument, so it ran in full first — a lone queued echo walked the transcript twice for a discarded result. Now guarded. Also drops a redundant scan: the candidate rows are the oldest send's row slice, so index 0 is representable in every one of them by construction and no longer needs computing.

Mutation evidence (all executed at ee94f1bc39)

Mutation Result
Off-by-one on the boundary index (index + 1 → index) 10 RED
Disable the per-send guard (pre-fix behavior) 2 RED — was 1 RED before the split
Over-apply the guard (nothing representable) 10 RED
Break only the pendingSendsAsMessages call site 1 RED
Adjacency break → continue 1 RED (was 0 RED pre-ee94f1bc)

The guard is fenced in both directions — under-applying and over-applying both go red — and the off-by-one class that shipped three times in this file is now pinned.

Desktop QA — before/after on one instance

Isolated PR-head Electron instance driven over Playwright CDP, fresh profile (window.api.repos.list() returned [] on first attach). Two real composer sends produced queued bubbles; a concatenated user row was written into the watched transcript; a third prompt was then queued after that glued row was already on screen.

  • With the fix: the third bubble survived.
  • After HMR to the oldest-only matcher: the third bubble vanished — the exact STA-4477 symptom.
  • After restoring the fix: it survived again.
  • 0 console errors (4 pre-existing warnings, none new).

Screenshots are in the QA comment below. Not reproduced: a live agent actually concatenating two fast TUI sends — the glued row was written into the watched transcript after two real sends. Mobile, Windows/WSL and SSH/Linux legs are out of scope for this change and were not exercised.

Precedent

Checked against mature implementations of the same optimistic-echo reconciliation problem. Three things line up:

  1. Applying one queue-wide anchor boundary to un-positioned optimistic rows is genuinely wrong, and none of them do it. Carrying the boundary per optimistic send is the correct formulation — so fix(native-chat): stop glued rapid sends from pinning queued bubbles #14663's candidate-set filtering was the defect and this PR's direction is right.
  2. Head-only, stop-on-mismatch traversal is the established idiom for walking a queue against a coalesced row. The break is precedent-aligned, not a judgment call.
  3. The low-pending early-out is the load-bearing optimization in every one of them, precisely because the steady state is an empty queue. That is what ee94f1bc restores. Memoizing the per-send eligible-row set would be the wrong shape — the transcript changes on nearly every token, so the hit rate would be ~0.

Perf (measured, 2000-message transcript, mean over 200 calls, 3 runs)

pre-ee94f1bc at head
1 pending (common case) 0.050–0.066 ms 0.020–0.021 ms
8 pending (PENDING_SEND_LIMIT) 0.29–0.71 ms 0.25–0.31 ms

~2.6× faster on the common path, unchanged at max queue depth. Both call sites are effect/useMemo-gated on [pending, messages] (NativeChatView.tsx:219-223, 291-294), so this recomputes when the transcript or pending set changes, not per render. Trivially bounded.

Readiness

  • Wire compatibility: no wire change. NativeChatPendingSend / NativeChatUserRow / NativeChatGluedUserRow appear in exactly three renderer files and never cross an IPC/RPC/preload surface.
  • Security / supply chain: no dependency, lockfile, binary, or script changes — all six changed files are under components/native-chat/.
  • Backcompat / data loss: no persisted schema, settings, or migration touched; this is client-local React state. No mobile-facing surface (the mobile equivalent lives in its own tree).
  • Unbounded growth: caches stay bounded (PENDING_SEND_LIMIT / COMMAND_MARKER_LIMIT = 8, plus LRU key bounding). New allocations are per-call locals bounded by open sends.
  • Cross-platform: pure TypeScript logic — no paths, shells, PTYs, or native modules.
  • Module split: forced, not scope creep. native-chat-pending.ts sat at 299 effective lines against a 300 cap, so any added line broke it. The moved block is byte-identical to the removed one and the remaining import is still legitimately used. max-lines was neither bumped nor disabled — I proved the gate is live by planting 80 code lines: error eslint(max-lines): File has too many lines (319). Maximum allowed is 300.

Gates

  • 73 files / 770 tests pass at head.
  • Merged tree materialized and run against current main — not just a clean merge-tree: 770 pass, 0 conflicts.
  • CI green against exactly ee94f1bc39: 44 pass, 1 skipping (e2e), 0 failures. MERGEABLE / CLEAN.
  • oxfmt: all changed files correctly formatted.

Residual risk / follow-ups (not blocking, not addressed here)

  • Pre-existing, out of scope: an image-only pending send (empty text) is continued rather than treated as a run-breaker, so it does not break glue adjacency between the text sends around it. This predates the PR and I did not adjudicate it fully — worth a follow-up ticket rather than a change here.
  • Architectural, well beyond STA-4477: the cleanest long-term shape is to mint a stable correlation id per optimistic send at submit time and have the transcript row carry it, or to coalesce queued sends into one optimistic entry before dispatch so the N→1 reconcile case never arises. Either would delete the matching, boundary, and ordering problems outright. Deliberately not attempted here.
  • Two review-agent rounds did not return a written verdict (they were starved under heavy machine load). Round 2 ran to completion and changed nothing, but I am recording that as "no written sign-off" rather than claiming a clean round I did not receive — the mutation battery, perf numbers, merged-tree run and lint-gate proof above are all my own executed evidence.

Verdict: no P0, no P1. Ready to merge.

…glue exports

Renaming the glue matcher's exports left this caller behind, and it crashed at
runtime after printing six result rows:

  TypeError: matchingNativeChatUserTexts is not a function

No gate caught it. config/scripts/** is in no tsconfig include and the file is
not a *.test.ts, so neither typecheck nor vitest ever loads it.

The empty-pending arm passes no pending sends, so the matcher takes its
empty-queue exit without ever reading the rows — which is also why the renderer
skips candidate-row construction entirely in that case. Escaping the row scan
directly keeps what this arm actually measures identical to before, rather than
fabricating per-row boundary sets that no production path builds.

@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: ea9d5998-1bec-44fc-aea4-3c987719b80a

📥 Commits

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

📒 Files selected for processing (7)
  • config/scripts/native-chat-live-session-benchmark.ts
  • src/renderer/src/components/native-chat/NativeChatInteractiveCard.test.tsx
  • src/renderer/src/components/native-chat/NativeChatView.tsx
  • src/renderer/src/components/native-chat/native-chat-command-marker.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-pending.ts

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

Comment on lines +52 to +55
// Why: the per-key array is capped at 8, but the KEY (paneKey\0agent\0sessionId,
// sessionId changes on every /clear) is ephemeral and was never evicted, so it
// grew one entry per (pane, session) for the renderer's whole life. LRU-bound
// the key count (mirrors the #7566 draft/attachment caches in this folder).

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Shorten the cache-history comment.

Lines 52-55 include historical detail that the code does not need. Keep one brief comment that states why scope keys need bounding.

Proposed change
-  // Why: the per-key array is capped at 8, but the KEY (paneKey\0agent\0sessionId,
-  // sessionId changes on every /clear) is ephemeral and was never evicted, so it
-  // grew one entry per (pane, session) for the renderer's whole life. LRU-bound
-  // the key count (mirrors the `#7566` draft/attachment caches in this folder).
+  // Bound scope keys because session IDs rotate after `/clear`.

As per coding guidelines: “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Why: the per-key array is capped at 8, but the KEY (paneKey\0agent\0sessionId,
// sessionId changes on every /clear) is ephemeral and was never evicted, so it
// grew one entry per (pane, session) for the renderer's whole life. LRU-bound
// the key count (mirrors the #7566 draft/attachment caches in this folder).
// Bound scope keys because session IDs rotate after `/clear`.

Source: Coding guidelines

Comment on lines +186 to +188
if (represented.has(entry.index) || entry.text.length === 0) {
continue
}

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Stop the glue run at an unresolved empty-text send.

continue skips an image-only pending send. A queue of first, image-only, and second can then match one firstsecond row and retire first and second as adjacent glue. The image send is an unmatched boundary.

Keep skipping already represented entries. Break when an unresolved entry has empty normalized text.

Proposed fix
-      if (represented.has(entry.index) || entry.text.length === 0) {
+      if (represented.has(entry.index)) {
         continue
       }
+      if (entry.text.length === 0) {
+        break
+      }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (represented.has(entry.index) || entry.text.length === 0) {
continue
}
if (represented.has(entry.index)) {
continue
}
if (entry.text.length === 0) {
break
}

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Follow-up: round 2 CLEAN, plus one real defect this PR had introduced (e57a3353)

All four review lanes have now reported. Two updates to my previous comment, one of them a correction.

e57a3353 — the rename left a broken caller, and no gate could see it

The readiness pass caught something I had missed. Renaming the glue matcher's exports left config/scripts/native-chat-live-session-benchmark.ts calling the deleted matchingNativeChatUserTexts / selectPendingIndicesRepresentedByUserTexts. I reproduced it: the benchmark prints six result rows and then dies.

TypeError: (0 , import_native_chat_pending_occurrence.matchingNativeChatUserTexts) is not a function
    at oldEmptyPending (config/scripts/native-chat-live-session-benchmark.ts:99:5)

Nothing in CI could have caught this. config/scripts/** appears in no tsconfig include, and the file is not a *.test.ts, so neither typecheck nor vitest ever loads it — the typecheck-blind-directory class, here extended from tests/ to config/scripts/. Developer-tooling impact only, no user-facing surface, which is why it is a P2 and not a merge blocker. But a rename that leaves a broken caller isn't what we'd ship, so it's fixed.

One thing worth recording about the fix. The obvious migration — map each row into the new {text, representablePendingIndices} shape — would have silently changed what this benchmark arm measures. That arm passes an empty pending queue, so the matcher takes its empty-queue exit and never reads the rows at all; fabricating a boundary set per row would have added per-row allocation that no production path performs (the renderer skips candidate-row construction outright when the queue is empty). Letting the row scan escape directly keeps the measurement identical to what it was before the rename. The benchmark now completes all 8 validated cases instead of dying after 6, with both escape assertions passing.

max-lines was a real constraint here too — the first version of the fix pushed the file to 302 against the 300 cap. It was solved by writing less code, not by bumping or disabling the rule.

Round 2: CLEAN

The second review round found nothing and changed nothing, which is what terminates the loop. Its central evidence is stronger than mutation testing alone: it materialized the pre-round-1 module alongside the current one and ran 40,000 randomized transcripts through both prunePendingSends and pendingSendsAsMessages — 0–6 messages with mixed roles and ~1-in-8 null timestamps, 0–4 pending sends, glue-prone texts, and all four anchor shapes (undefined, null, real id, paged-out id).

Zero divergence. The round-1 refactor is behavior-identical to the fix it sits on. The harness was proven non-vacuous — four planted mutations in gluedCandidateRows all went red, including both off-by-one directions.

Round 2's mutation table also adds a useful control: removing the low-pending early-out entirely yields 0 red, confirming it is behavior-neutral, while widening it to <= 1 yields 7 red, confirming the boundary itself is load-bearing.

Correction: the empty-text continue is not a latent defect

My previous comment listed this as an unadjudicated follow-up candidate. Having now worked it through, that was too cautious and I'm withdrawing it.

The two review rounds initially disagreed — one called it correct by design, the other a reachable latent defect of the same class. Both are half right, and the deciding fact is monotonicity. An empty-text (attachment-only) pending entry is skipped rather than breaking the glue run, so its own boundary is never checked. But pending boundaries are minted as session.messages.at(-1) at send time (NativeChatView.tsx:239), which is monotonically non-decreasing. So if the attachment-only send lands after the glued row, every send queued after it does too — and those entries do get their boundary checked and do break the run. The asymmetry exists in the code but is unreachable in production, and the skipped entry is never retired by glue in any case. No follow-up ticket needed.

The same monotonicity note applies to the new break-coverage test: its fixture uses a deliberately non-monotone boundary, so read it as a synthetic stress of the break rather than a reproduction of a field scenario. That is the right way to test that line, but it shouldn't be mistaken for a real-world ordering.

Remaining known gaps, stated plainly

  • No test drives the per-entry boundary through the paged-out / timestamp-fallback arm (native-chat-pending.ts:103-109), which is the arm most exposed to renderer-vs-host clock skew under SSH. Failure direction there is conservative — a duplicate bubble lingers, never a vanished prompt.
  • The clock-skew trade is a judgment call worth naming rather than burying: under a skewed remote clock, a glued pair that previously retired now does not. That lost leniency was the STA-4477 bug, and glue now fails exactly where the exact-match path already fails, which is the PR's own stated invariant. The failure direction moves from data loss to visual noise. A slack constant for this hazard already exists on adjacent paths, but applying it here would mean changing a predicate shared with the exact path — which risks re-opening the class this file has already shipped twice. That is a separate ticket with its own regression surface, not an STA-4477 edit.

Final state

  • CI green on e57a335327: 44 pass, 1 skipping, 0 failures. MERGEABLE / CLEAN.
  • 73 files / 770 tests green; merged-tree run against current main green.
  • oxlint and oxfmt clean on every changed file, both gates proven live by planted violations.

Verdict unchanged: no P0, no P1. Ready to merge.

@brennanb2025
brennanb2025 merged commit 4a6de51 into main Aug 17, 2026
45 checks passed
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Final review-loop status (post-merge, 4a6de51ad8)

Merged at head e57a335327, so all three commits landed. I re-verified against merged main (7b4e10b1): 73 files / 770 tests green, and the previously-broken benchmark script runs all 8 validated cases. No dangling references to the removed exports remain anywhere in the tree.

Nothing is outstanding that should have blocked this merge. One systemic gap it landed with is worth a follow-up ticket — see the last section — but it is a tooling-visibility issue, not a defect in the shipped behavior.


Rounds run, and the stopping condition

Two review rounds, plus a readiness checklist at the start and again at the end, an independent precedent check, and a desktop QA lane.

  • Round 1 — found three real defects. Pushed ee94f1bc39.
  • Round 2 — found nothing and changed nothing. That was the stopping condition: the loop only terminates on a round that both finds nothing and pushes nothing, because the characteristic failure of these loops is a round fixing a defect its predecessor introduced.
  • After round 2, I fixed one defect the readiness pass had found that neither review round looked at (e57a335327).

Did any round introduce a defect that a later round had to fix?

No — and I checked this deliberately rather than assuming it. This is the notable contrast with the sibling PR's loop, where four of seven rounds were cleaning up the previous round's own fix.

Round 2 did not take round 1's changes on faith. It materialized the pre-round-1 module alongside the post-round-1 one and ran 40,000 randomized transcripts through both prunePendingSends and pendingSendsAsMessages — 0–6 messages with mixed roles and roughly one-in-eight null timestamps, 0–4 pending sends, glue-prone text alphabet, and all four boundary-anchor shapes (absent, null, real row id, paged-out id). Zero divergence. Round 1's refactor is behavior-identical to the fix it sits on.

That harness was itself proven non-vacuous: four mutations planted into the refactored function all went red, including both off-by-one directions on the boundary index.

There was one near-miss of exactly this class, caught before it was committed — mine, not a review round's. My first version of the benchmark fix mapped every transcript row into the new boundary shape. That would have compiled, run, and looked correct while silently changing what the benchmark measures, because that arm passes an empty queue and the matcher never reads the rows at all. It also pushed the file to 302 lines against a 300 cap. Both were caught pre-commit and the fix was rewritten to escape the row scan directly, which keeps the measurement identical to before the rename.

What the test commit actually unmasked

This is the most useful thing for a future reader, and the honest framing is two different problems, not one.

1. The fix's central semantic had zero coverage. The break in the matcher is the whole idea of the fix: a send the transcript row predates ends the glue run rather than being skipped over, because physical adjacency on one input line is what makes sends glue. Swapping that break for a continue passed all 62 tests. Nothing exercised a queue whose middle entry is unrepresentable. With continue, a queue ['alpha','beta','gamma'] where a row can represent alpha and gamma but not beta glues "alpha gamma" across the gap and retires two live prompts — reintroducing the exact data-loss class the PR exists to close, via a one-word edit no test would have caught. I verified this both directions myself: 62/62 green against the original tests, 1 red against the new one.

2. An assertion that was unreachable under the failure it existed to catch. The mixed-age test asserted prunePendingSends and then pendingSendsAsMessages inside a single it. Removing the guard failed the first assertion, so the second one never executed. To be precise, it was not passing vacuously in the green state — both ran normally when things worked. The problem was on the failure path: the render assertion was masked by the prune assertion, and the render path is the one that makes a queued bubble visually vanish. A regression breaking both would have surfaced as a single prune failure, and the render breakage would only have appeared after a second fix-and-rerun cycle. Split into two tests; the same mutation now yields 2 reds instead of 1.

Mutation counts at the merged head: removing the per-send guard → 2 red; breaking only the render call site → 1 red (the two consumers are independently guarded); over-applying the guard → 10 red; off-by-one on the boundary index in either direction → 10 red; break→continue → 1 red.

Readiness checklist — start and end

Start (against the original 9fef6a69): SHIP. No P0, no P1. One P2: the export rename left config/scripts/native-chat-live-session-benchmark.ts calling two deleted functions. Proven at runtime — six result rows, then TypeError: ... is not a function. Also flagged a duplicated computation of the oldest send's row set, which round 1 independently eliminated.

End (re-verified against ee94f1bc39): verdict unchanged, SHIP. It confirmed round 1's refactor was behavior-identical by reading it directly, and confirmed the new test is load-bearing for the break. The benchmark P2 was still open at that point, which is what prompted e57a335327.

Both passes agreed on the rest: no wire surface, nothing persisted, no mobile-facing surface, no dependency/lockfile/binary/script changes, no new casts or unbounded growth, and nothing platform-specific.

Prior art — is this a standard senior-level fix?

Yes. Nothing in it is hacky. Checked generically against mature implementations of the same optimistic-echo reconciliation problem; three things line up:

  1. Applying one queue-wide anchor boundary to un-positioned optimistic rows is simply wrong, and no mature implementation does it. Carrying the boundary per optimistic send is the standard formulation — so the earlier candidate-set filtering was the defect, and this PR's direction is right.
  2. Head-only, stop-on-mismatch traversal is the established idiom for walking a queue against a coalesced row. The break is precedent-aligned, not a local judgment call.
  3. The low-pending early-out is the load-bearing optimization in these implementations, precisely because the steady state is an empty queue. That is what round 1 restored. Memoizing the per-send eligible-row set would have been the wrong shape — the transcript changes on nearly every token, so hit rate would be near zero.

The cleaner long-term shape is architectural, not a fix-level change: mint a stable correlation id per optimistic send at submit time and have the transcript row carry it, or coalesce a queued run into a single optimistic entry before dispatch so the many-to-one reconcile never arises. Either deletes the matching, boundary, and ordering problems outright. Far outside this ticket; deliberately not attempted.

Performance at PENDING_SEND_LIMIT = 8

Confirmed bounded. No quadratic, no allocation blowup.

Shape is O(open × messages) with open hard-capped at 8. Measured on a 2000-message transcript, mean over 200 calls, three runs:

before the round-1 guard merged
1 pending (common case) 0.050–0.066 ms 0.020–0.021 ms
8 pending (the cap) 0.29–0.71 ms 0.25–0.31 ms

~2.6× faster on the common path, unchanged at the cap. Both consumers are effect/memo-gated on the transcript and pending set, so this recomputes per transcript change, not per render.

One measurement caveat worth recording: a single 8-pending sample read 0.41 ms and looked like a regression. Three runs showed it was JIT/GC noise. A separate 3000-message reading of 13.5 ms/call was cold-JIT and collapsed to ~1 ms warmed. Neither was real — reporting either would have been a false finding.

Electron QA — what was validated, and what was blocked

Driven over Playwright CDP on an isolated PR-head instance with its own profile, CDP port and renderer port; ownership verified before capturing anything. Screenshots are in the QA comment above.

Validated: two real composer sends produced queued bubbles; after a glued transcript row landed, a third prompt queued after that row survived with the fix, vanished after hot-reloading to the oldest-only matcher, and returned when the fix was restored. That is the STA-4477 symptom reproduced and closed on a live renderer. 0 console errors (4 pre-existing warnings, none new).

Blocked, stated plainly — I do not claim this coverage:

  • Live agent glue was never produced. No running agent concatenated two fast sends onto one input line. The glued row was written into the watched transcript after two genuine composer sends. The reconciliation path is real; the mechanism that produces a glued row in the field is not what was exercised.
  • The hot reload cleared the in-memory pending queue, so the restored-fix confirmation is a resend, not the same cache entry surviving a round trip.
  • Not exercised at all: mobile, Windows/WSL, SSH/Linux, and a real assistant reply arriving after glue. Those are out of scope for a desktop renderer change.

Deliberately not done

  • No clock-skew slack constant. A slack value for this hazard already exists on adjacent paths, but applying it here means changing a predicate shared with the exact-match path. Loosening exact matching risks re-opening the over-retirement class this file has already shipped twice. Separate ticket with its own regression surface.
  • No last-rendered duplicate suppressor. The precedent pairs one with stop-on-mismatch traversal, but the exact-match path already fills that role here and runs first, so adding one speculatively would have been unjustified scope.
  • No architectural change (correlation id at submit, or coalesce-before-dispatch), per above.
  • No fix for the empty-text continue. The two rounds initially disagreed on this; I adjudicated it and withdrew it as a defect. An attachment-only pending entry is skipped without its own boundary check, but boundaries are minted from the last transcript message at send time and are therefore monotonically non-decreasing — so any send queued after it also post-dates the row and does break the run. Unreachable in production, and the skipped entry is never retired by glue regardless. No ticket needed.
  • No unrelated cleanups. Every changed file is in this fix's own blast radius.

Residual risk this merged with

  1. config/scripts/** is invisible to every gate — this will recur. It is in no tsconfig include and its files are not tests, so neither typecheck nor the test runner ever loads them. That is why a rename shipped a runtime TypeError with fully green CI. The specific caller is fixed, but the class is not, and the next rename of any renderer export will reproduce it silently. This is the one item I would open a follow-up ticket for.
  2. No test drives the paged-out / timestamp-fallback boundary arm, which is the arm most exposed to renderer-vs-host clock skew under SSH. Failure direction there is conservative — a duplicate bubble lingers, never a vanished prompt.
  3. A real behavior trade under clock skew: a glued pair that previously retired now does not, when the remote clock trails far enough. That lost leniency was the bug — it came from applying the oldest send's boundary queue-wide — and glue now fails exactly where exact matching already fails, which is the fix's own stated invariant. The failure direction moved from data loss to visual noise. That is the right trade, but it is a trade, not a free win.
  4. The new break test's fixture is deliberately non-monotone, which production cannot produce. It is a synthetic stress of that one line — correct for the purpose, but should not be read as a reproduction of a field scenario.

Honest summary

The production fix was already correct when I picked it up. Neither round found a behavioral defect in it, and I want to be clear about that rather than imply the loop rescued it. What the loop actually produced was test integrity: the fix's defining semantic was completely unpinned, and its render-path assertion was unreachable under failure. Plus one broken caller that no gate in this repository could see.

paidaxingyo666 pushed a commit to paidaxingyo666/Manta that referenced this pull request Aug 21, 2026
…tching (STA-4477) (stablyai#14935)

* fix(native-chat): enforce each pending send's own boundary in glue matching

Glue matching filtered candidate rows against the OLDEST still-open send and
then matched the entire open queue against them. A prompt queued after a glued
row landed could therefore be judged "already delivered" by that older row and
pruned — the queued prompt disappeared with no bubble and no transcript turn.

Each send now carries its own transcript boundary into the match:
`gluedCandidateRows` tags every candidate row with the set of pending indices
it actually landed after, and the matcher stops a run at the first send the row
predates rather than skipping over it (adjacency is what makes a row glue).
Exact single matches still belong to the occurrence path, unchanged.

`native-chat-pending.ts` sat at 299 of its 300 effective-line budget, so the
slash-command marker cache — a separate rule that never took part in pending
pruning — moves verbatim to `native-chat-command-marker.ts`. Pure move: no
behavior change, imports only. (max-lines is never bumped or disabled.)

Refs STA-4477. Original PR stablyai#14663.

* test(native-chat): cover the glue adjacency break and unmask the render path

The `break` on a send the row cannot represent is the fix's central semantic
choice, and swapping it for `continue` was passing the whole suite: nothing
exercised a queue whose middle send is unrepresentable. Add that case.

The mixed-age case also asserted both call sites in one `it`, so a prune-path
failure masked the render-path assertion — and the render path is the one that
makes a queued bubble visually vanish. Split it.

Skip the per-send boundary scans when fewer than two sends are open: the glue
matcher already returns nothing there, so a lone queued echo was walking the
transcript twice per render for a discarded result.

* fix(native-chat): migrate the live-session benchmark off the renamed glue exports

Renaming the glue matcher's exports left this caller behind, and it crashed at
runtime after printing six result rows:

  TypeError: matchingNativeChatUserTexts is not a function

No gate caught it. config/scripts/** is in no tsconfig include and the file is
not a *.test.ts, so neither typecheck nor vitest ever loads it.

The empty-pending arm passes no pending sends, so the matcher takes its
empty-queue exit without ever reading the rows — which is also why the renderer
skips candidate-row construction entirely in that case. Escaping the row scan
directly keeps what this arm actually measures identical to before, rather than
fabricating per-row boundary sets that no production path builds.
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
…tching (STA-4477) (stablyai#14935)

* fix(native-chat): enforce each pending send's own boundary in glue matching

Glue matching filtered candidate rows against the OLDEST still-open send and
then matched the entire open queue against them. A prompt queued after a glued
row landed could therefore be judged "already delivered" by that older row and
pruned — the queued prompt disappeared with no bubble and no transcript turn.

Each send now carries its own transcript boundary into the match:
`gluedCandidateRows` tags every candidate row with the set of pending indices
it actually landed after, and the matcher stops a run at the first send the row
predates rather than skipping over it (adjacency is what makes a row glue).
Exact single matches still belong to the occurrence path, unchanged.

`native-chat-pending.ts` sat at 299 of its 300 effective-line budget, so the
slash-command marker cache — a separate rule that never took part in pending
pruning — moves verbatim to `native-chat-command-marker.ts`. Pure move: no
behavior change, imports only. (max-lines is never bumped or disabled.)

Refs STA-4477. Original PR stablyai#14663.

* test(native-chat): cover the glue adjacency break and unmask the render path

The `break` on a send the row cannot represent is the fix's central semantic
choice, and swapping it for `continue` was passing the whole suite: nothing
exercised a queue whose middle send is unrepresentable. Add that case.

The mixed-age case also asserted both call sites in one `it`, so a prune-path
failure masked the render-path assertion — and the render path is the one that
makes a queued bubble visually vanish. Split it.

Skip the per-send boundary scans when fewer than two sends are open: the glue
matcher already returns nothing there, so a lone queued echo was walking the
transcript twice per render for a discarded result.

* fix(native-chat): migrate the live-session benchmark off the renamed glue exports

Renaming the glue matcher's exports left this caller behind, and it crashed at
runtime after printing six result rows:

  TypeError: matchingNativeChatUserTexts is not a function

No gate caught it. config/scripts/** is in no tsconfig include and the file is
not a *.test.ts, so neither typecheck nor vitest ever loads it.

The empty-pending arm passes no pending sends, so the matcher takes its
empty-queue exit without ever reading the rows — which is also why the renderer
skips candidate-row construction entirely in that case. Escaping the row scan
directly keeps what this arm actually measures identical to before, rather than
fabricating per-row boundary sets that no production path builds.
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