Skip to content

fix(desktop): defer approval replay until session rebind - #137641

Open
ytahml wants to merge 1 commit into
NousResearch:mainfrom
ytahml:codex/desktop-approval-reconnect
Open

ytahml wants to merge 1 commit into
NousResearch:mainfrom
ytahml:codex/desktop-approval-reconnect

Conversation

@ytahml

@ytahml ytahml commented Oct 13, 2026 •

Copy link
Copy Markdown

What does this PR do?

After a Desktop WebSocket outage exceeds the backend's orphan grace period, gateway.ready can replay approval.pending against a reclaimed runtime before session.resume rebinds the conversation. That produces a 4001 request on reconnect.

Stop replaying approvals on socket readiness. Keep the existing successful resume/activate snapshot restoration and session.info refresh paths, so live pending approvals still restore. This only changes Desktop replay timing; approval decisions and backend reclamation are unchanged.

Upgrade-First Verification and Scope

Before developing this patch on 2026-10-13, I backed up the installation and ran the official hermes update --yes: v0.21.5+7251.ge473f5a → v0.21.7+279.g63d0b94. Revision 63d0b94120027d487c9a5235b9ec64b696af1b26 was the latest upstream main at investigation and filing. Desktop was rebuilt and installed, the gateway was restarted, and the running revision was checked. There were no local source modifications at that point.

The upgrade already included upstream heartbeat fix 1dfef400a7: throttled hidden-window timers no longer cause a healthy socket to be closed when the previous ping was acknowledged. A deterministic throttled-timer regression failed on the old version and passed on the upgraded version. That fix belongs to upstream and is not part of this PR.

After the upgrade, initial smoke testing succeeded. A focused reconnect regression against unpatched 63d0b94 then reproduced the remaining approval-ordering defect: three simulated reconnects issued three retired-runtime queries plus three rebound-runtime queries. With this patch, only the three rebound IDs are queried, and their pending approvals still restore. The Electron test separately verifies recovery after a 25-second simulated outage with the patch applied.

The distinction is between preventing false heartbeat disconnects (already fixed upstream) and avoiding an approval query against a retired runtime when reconnecting (this PR). The post-upgrade evidence is a controlled reproduction of the second problem; it does not claim that the original spontaneous-disconnect pattern persisted, or that physical sleep/wake was tested.

Related Issue

Fixes #137640

Related: #101531 handles mounted warm-session reattachment. This change addresses the earlier gateway.ready approval query in a separate path.

Type of Change

  • Bug fix

Changes Made

  • Remove the gateway.ready replay branch and unused active-session argument from approvalReplaySessionId; update its caller and existing tests.
  • Add a regression using the real prompt store and routing/guard helpers across three reconnects. Before the fix, it made six queries including three retired runtime IDs; after the fix, only the three rebound IDs are queried and pending approvals are restored.
  • Add an Electron test with a real backend and loopback mock model: disconnect for 25 seconds, require a fresh runtime, retain the transcript, send another message, and assert zero stale approval rejections.

How to Test

From apps/desktop with the repository's JS dependencies installed:

npm test -- src/store/prompts-reconnect.test.ts src/store/prompts.test.ts src/lib/gateway-events.test.ts src/store/runtime-gone.test.ts src/store/session-request-router.test.ts src/lib/json-rpc-gateway-heartbeat.test.ts
npm run typecheck
npx --no-install tsc -p tsconfig.e2e.json --noEmit
npm run build
npx --no-install playwright test e2e/approval-reconnect.spec.ts --workers=1 --reporter=list

For a PM-managed installation, point HERMES_DESKTOP_PYTHON at the existing managed Python interpreter when running the E2E test. The test creates isolated application state and uses synthetic credentials for its loopback mock provider.

Validated on macOS 15.7.7 arm64 / Python 3.14.7:

  • Six focused Vitest files: 86 passed.
  • Renderer/Electron and E2E TypeScript checks: passed.
  • Changed-file ESLint: zero errors or warnings; git diff --check: passed.
  • python3 scripts/check --staged: 12 checks passed, zero blocking or advisory code-health findings.
  • Desktop build: passed. Electron reconnect E2E: 1 passed, approximately 39 seconds.
  • A rebuilt local application resumed a pre-existing conversation and completed a follow-up turn.

The contribution files match the tested local patch byte for byte. Validation scope: the full Python test suite, Linux/Windows execution, and physical sleep/wake behavior were not tested; this patch changes only TypeScript in Desktop.

Checklist

  • Read the contributing guide; conventional commit and one focused fix.
  • Searched existing issues/PRs; related work is linked above.
  • Added behavior regressions and verified the old replay ordering fails.
  • Relevant JS tests, type checks, build, and repository blocking lint checks pass.
  • Tested on macOS arm64; no platform-specific production logic added.
  • Documentation, configuration schema, tool schema, dependencies and architecture changes: N/A.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Oct 13, 2026

@ehz0ah ehz0ah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approval conclusion

I reviewed the full change at c265f1c and found no blocking issue.

This removes the stale pre-rebind approval query at the right boundary. Previously, gateway.ready could ask for pending prompts before the routed session was rebound. The reconnect path now relies on session.resume and the existing resume, activate, session-info, and JSON-RPC open-request recovery paths, so pending approvals are restored from the current runtime instead of an old route. The caller changes and reconnect coverage match that behavior.

I ran git diff --check and 42 focused Vitest cases covering gateway event routing and JSON-RPC replay. All passed. I did not rerun the Electron end-to-end case or the full prompt-store suite because the complete Desktop dependency graph is not available in this checkout.

GitHub shows no hosted checks for this head. The change looks good to me, but I would wait for CI before merging. I am posting this as a comment review because GitHub does not allow this account to submit a formal approval without explicit repository access.

@ytahml

ytahml commented Oct 13, 2026

Copy link
Copy Markdown
Author

Thanks for reviewing this, @ehz0ah, and for running the focused tests. Locally, all 86 related Vitest tests and the Electron reconnect E2E test passed. The E2E test uses a real backend and a loopback mock model, and verifies recovery after a 25-second simulated disconnect, including a fresh runtime ID, preserved transcript, and no stale approval rejections.

We're currently waiting for a maintainer to approve the GitHub Actions run so CI can execute. I agree that we should wait for CI results before merging.

This branch has not been deployed

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop queries pending approvals before reconnect session rebind

3 participants