Skip to content

fix(native-chat): show a pending approval's controls on remote and phone clients after temporary read failures - #35339

Merged
AmoabaKelvin merged 1 commit into
mainfrom
sta-10928-retry-pending
Oct 28, 2026
Merged

AmoabaKelvin merged 1 commit into
mainfrom
sta-10928-retry-pending

Conversation

@AmoabaKelvin

@AmoabaKelvin AmoabaKelvin commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​45 $\color{#cf222e}{\Huge{\mathbf{−}}}$​19 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​26
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​30 $\color{#cf222e}{\Huge{\mathbf{−}}}$​14 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​16

ELI5

When an agent asks for approval, a phone or a desktop connected to another computer has to make a second, separate request to load that approval. If the request timed out twice in a row, for example on a slow connection, the client gave up on it for good. The chat kept moving, but the approval never got its Allow / Deny buttons until the chat was reopened. Now a client that hits a temporary failure keeps retrying, waiting a bit longer each time, and the buttons show up once a read gets through.

What Changed

Before: on a phone, or on a desktop or web client paired to a remote host, two temporary agentSession.currentRequests failures for the same decision left the approval or question without controls. The host still showed them. Local desktop chats go through in-process IPC and were effectively unaffected.

After: the controls appear as soon as a read succeeds, and nothing reopens the transcript subscription.

Mechanism, in the shared reader that desktop, web and phone all use (src/shared/agent-session-current-requests-reader.ts):

  • A read that fails in transit (the RPC rejects) now re-reads the same version. Retries use the stream's reconnect scheduler: 750 ms, doubling up to 30 s, reset when a read completes or the reader is invalidated.
  • Two kinds of failure still stop the reader: an answer that breaks the contract (bad JSON, inadmissible items, a bad cursor or owner), and a final journal refusal. Those are the only failures it reports to the live read now. Retried failures stay silent, so a refusal from a read that later succeeds doesn't leave the chat stuck showing an error.
  • structured-agent-session-live-read.ts loses its "reopen the subscription once per failing version" recovery. A failed decision read no longer tears down a healthy transcript stream.

Why

Root cause: the reader recorded a version as attempted before reading it and never read that version again after any failure. The live read's only recovery was one subscription reopen per version, so a second temporary failure left the decision unread until its version changed.

The fix lives in the reader because that's where the "never read this version again" rule was. One change covers desktop, web and phone, since all three run the same shared live read. Alternatives I didn't take:

The ticket's other symptom, the phone retiring its transcript subscription, is already gone on main. #29007 deleted mobile/src/session/mobile-structured-transcript-read.ts and moved the phone onto the same shared live read. I confirmed it in the real run below: on main the phone transcript kept updating after the decision read gave up.

No wire change: same RPC, same params, same answers. Mixed client and host versions behave as before.

Linked Issue

No GitHub issue. Linear: STA-10928, a release-scan finding against #29121, which introduced the behaviour.

Visual Proof

The setup is real on both sides. A hidden dev desktop host built from this branch runs a real Claude chat in "Ask for approval" mode. Two real clients are paired to it: a second dev desktop connected as a remote runtime, and the released Orca Mobile Android 0.0.59 (OTA shell) on an emulator, rendering the page the host serves. To make the slow link repeatable, a temporary host patch (not in this PR) holds the next N agentSession.currentRequests replies from remote clients for 20 s, past both clients' 15 s RPC timeout.

Paired desktop client, main. With 4 held replies, the client's reads of the approval's version timed out twice. After that it never read the version again; the host log shows no further reads for 5+ minutes. The host has the controls; the client shows only "Working…":

Host (local) Paired client (main)
host with controls client without controls

Paired desktop client, this branch. With 6 held replies, the approval's version failed six times in a row. Retries came 0.75 s, 1.5 s, 3 s, 6 s and 12 s after each timeout. The seventh read succeeded, Allow appeared on the client two seconds later, and there was no subscription reopen. Clicking Allow on the client wrote the file:

client with controls

Android phone. On main, with 4 held replies, the phone's two reads of the approval's version timed out and it stopped reading, leaving only "Working…" (the Allow/Deny text visible is from older, resolved rows). On this branch, with 8 held replies, the version failed seven times; the eighth read succeeded and the card appeared. Tapping Allow on the phone wrote phone3.txt:

Phone, main Phone, this branch After tapping Allow
phone without controls phone with controls phone after allow

Testing

  • Real repro on macOS, as described above: before on main and after on this branch, for both a paired desktop client and an Android phone, each ending with the approval answered from the client.

  • structured-agent-session-read-transport.test.ts: replaced the "reopens once" test with two tests. One shows that temporary failures are re-read without reopening the stream and without reporting an error. The other shows that a contract-breaking answer isn't re-read. Both fail on main.

  • Ran the dependent tests agent-session-current-requests.test.ts and structured-agent-session-current-requests.test.ts, plus the changed-file code-quality and React Doctor gates.

  • iOS simulator: not exercised. CoreSimulator hung on boot and create on this machine during the run. iOS runs the same page bundle as Android.

  • Linux, Windows and SSH: not run. The change is platform-neutral shared TypeScript with no transport or path code.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

AI Disclosure

Review

Two review rounds, with Codex and Claude reviewing each:

  • Round 1: Codex found nothing. Claude found a P2, fixed here: a refusal from a read that later succeeded would have stayed on screen. Only failures the reader gives up on are reported now.
  • Round 2: Codex found nothing. Claude raised three P3s, left as they are:
    • A host error that will never clear is re-read every 30 s while the chat stays open. The cost is one small RPC per 30 s, and that's the same policy the transcript stream uses.
    • Backoff grown on one version carries over to the next version's first retry.
    • A host test's error callback no longer sees retried failures.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

  • Mobile: the page bundle ships with the desktop, so phones get this through a desktop update. The phone app itself doesn't change.
  • Performance: at most one small read in flight per open chat, at most once every 30 s after repeated failures. Retries stop when the stream drops, reconnects or the chat closes.
  • Remote/SSH: no wire change, so older and newer hosts and clients mix as before.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…lure

A failed agentSession.currentRequests read marked its version as tried for good, and the live read only reopened the subscription once per version. Two slow or dropped reads in a row left a pending approval or question without controls on remote desktop, web and phone clients until the decision changed.

The reader now re-reads a version whose read failed in transit, with the stream's reconnect backoff, and stops only when the answer breaks the contract or the host refuses for good. The transcript subscription is no longer reopened for a decision-read failure.
@greptile-apps

greptile-apps Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change reaches the shared native-chat read workflow across desktop, web, and phone clients.

The PR appears safe to merge, with a non-blocking gap in how desktop chats surface terminal approval-read errors.

Findings

  1. P2 Approval read errors disappear ▶

Summary

Moves temporary approval-read retries into the shared reader without reopening the transcript stream.

  • Pending approval controls appear after temporary read failures.

Acknowledged by AmoabaKelvin in the PR description:

  • Permanent host errors continue retrying every 30 seconds; this matches the transcript retry policy.
  • Backoff from one version carries into the next version’s first retry.
  • A host test’s error callback no longer receives retried failures.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Observe approval version] --> B[Read current requests]
  B -->|Temporary failure| C[Wait with capped backoff]
  C --> B
  B -->|Valid reply| D[Apply approval controls]
  B -->|Broken reply or final refusal| E[Report terminal failure]
  F[Reader invalidated] --> G[Cancel pending retry]
Loading

Reviews (1) · Last reviewed commit: "fix(native-chat): retry a pending decisi..." · Reviewed by Greptile

@coderabbitai

coderabbitai Bot commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ee2f5170-c547-4407-b00d-57131482ddf3
Commits

Reviewing files that changed from the base of the PR and between 7b3fb54 and 8b8526b.

Files selected for processing: 3
  • src/renderer/src/components/native-chat/structured-agent-session-read-transport.test.ts
  • src/shared/agent-session-current-requests-reader.ts
  • src/shared/structured-agent-session-live-read.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

The current-request reader retries eligible read failures for the same desired version. It reports final refusals and failures after receiving a response without a marker. A complete validated response resets the retry scheduler, and invalidation disposes it. Live-read handling no longer reopens the subscription for current-request read failures. Tests cover successful completion after temporary failures and confirm that invalid JSON is not reread.

Priority: Normal

Merge Risk: Minimal · up to 8b852

The reported forbidden-refusal concern does not block this change. No actionable merge-blocking issue remains after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the native-chat fix and its user-facing effect: showing pending approval controls after temporary read failures on remote and phone clients.
Description check Passed The description is complete and focused. It explains the user impact, technical mechanism, rationale, alternatives, testing, visual proof, platform considerations, and known limitations. The Linked Is…
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.
Full details: Description check

Explanation

The description is complete and focused. It explains the user impact, technical mechanism, rationale, alternatives, testing, visual proof, platform considerations, and known limitations. The Linked Issue section provides Linear issue STA-10928 but does not include an actual issue link.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 3 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@AmoabaKelvin
AmoabaKelvin merged commit 45364ef into main Oct 28, 2026
43 checks passed
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