Skip to content

fix(vscode): re-sync session or reset with announcement on ACP restart (#1719) - #1791

Open
rishu685 wants to merge 2 commits into
Nano-Collective:mainfrom
rishu685:fix/acp-restart-session-sync
Open

rishu685 wants to merge 2 commits into
Nano-Collective:mainfrom
rishu685:fix/acp-restart-session-sync

Conversation

@rishu685

Copy link
Copy Markdown
Contributor

Fixes #1719

Problem

When the ACP backend process restarts or crashes/reconnects, the webview and client state could diverge from the server session: previously active sessions were not automatically resumed, and if a fresh session was created, the webview could retain stale conversation state without notifying the user.

Solution

  1. Track lastActiveSessionId: Preserve the active session ID when connection drops, before restarting the ACP process, or when setConnection is called in NanocoderAcpClient.
  2. Re-sync or Announce Reset: During _initializeSessionIfReady(), if a lastActiveSessionId existed, attempt to resume it on the restarted backend.
  3. Fallback and Announcement: If resuming the previous session fails (e.g. backend process was fresh and lost unpersisted session state), reset the webview session state and post an explicit notification message informing the user that the Nanocoder agent process was restarted in a fresh session, before creating a new session.
  4. Tests: Added tests verifying lastActiveSessionId tracking and auto-resume / fallback announcement on restart.

@github-actions github-actions Bot added the area:vscode VS Code extension and host integration label Oct 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 1 important, 2 nits

@rishu685 — there is a blocking item below.

The PR resolves the resume-on-restart bug described in #1719 by tracking lastActiveSessionId on NanocoderAcpClient and adding a resume/fallback path in ChatWebviewProvider._initializeSessionIfReady. The core change is correct and well-scoped, with targeted tests for both the bookkeeping and the two _initializeSessionIfReady branches. However, the PR is missing a changeset — the project requires one for user-facing changes (existing vscode-plugin changesets in .changeset/ follow this convention), and that is a contributor-hygiene blocker per the rubric.

🔴 blocking · changeset · .changeset/

This is a user-facing bug fix in the VS Code extension and needs a .changeset/*.md entry. CONTRIBUTING.md says "Every user-facing PR includes a .changeset/*.md file", and the existing convention is to target @nanocollective/nanocoder for vscode-plugin changes (see vscode-new-chat-abandons-pending-session.md, vscode-prompt-queue.md, etc.). Without one, the change has no release note and release-prepare will leave it out of the changelog.

🟠 important · tests · plugins/vscode/src/chat-webview-provider.spec.ts:218

Both new _initializeSessionIfReady tests monkey-patch provider.resumeSession to a stub that returns a boolean without invoking the real implementation. The integration between _initializeSessionIfReady and the real ChatWebviewProvider.resumeSession method (which posts clear/sessionLoaded, resets plan/artifacts, discards queued prompts, and awaits _acpClient.resumeSession) is therefore not exercised. If the real method silently stopped calling _acpClient.resumeSession, or stopped posting sessionLoaded, both tests would still pass. Recommend one test that lets the real resumeSession run end-to-end against a stubbed _acpClient.resumeSession so the wiring is actually proved.

⚪ nit · correctness · plugins/vscode/src/chat-webview-provider.ts:503

If the provider's new resumeSession() method throws (e.g. vscode.window.showErrorMessage inside _acpClient.resumeSession rethrows, or one of the pre-try calls like _discardQueuedPrompts throws), the exception propagates out of _initializeSessionIfReady, the !resumed fallback is skipped, and the user sees the generic Failed to initialize session: … toast instead of the "restarted in a fresh session" announcement. With the current _acpClient.resumeSession implementation this is unreachable, but a defensive try/catch around the await this.resumeSession(previousSessionId) call would make the UX invariant — if the backend cannot resume, we always tell the user we are on a fresh session — robust against future changes to the underlying client.

⚪ nit · design · plugins/vscode/src/acp-client.ts:28

lastActiveSessionId has both a public set lastActiveSessionId(id) setter and a separate clearLastActiveSessionId() method for the trivial case of setting it to undefined. The setter is used by extension.ts (manual restartAcp); the clear-method is used by chat-webview-provider.ts. Two APIs for the same thing invite drift (e.g. one caller uses the setter with undefined and the other uses the method). Either collapse to one (the setter alone is enough — acpClient.lastActiveSessionId = undefined reads cleanly) or document why both exist.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:needs-work nc-review found blocking findings label Oct 16, 2026
@rishu685

Copy link
Copy Markdown
Contributor Author

Thank you for the review! All review feedback has been addressed in commit d70132d:

  1. Changeset: Added .changeset/vscode-acp-restart-session-sync.md targeting @nanocollective/nanocoder: patch documenting the session sync and restart fallback announcement.
  2. Integration Tests: Updated chat-webview-provider.spec.ts so provider.resumeSession runs end-to-end against a stubbed acpClient.resumeSession, verifying that clear (with isLoading: true), sessionLoaded, and connection status notifications are properly posted.
  3. Defensive Error Handling: Wrapped resumeSession in a try/catch block during _initializeSessionIfReady so unexpected errors gracefully trigger the fallback reset and fresh session announcement.
  4. API Cleanliness: Removed redundant clearLastActiveSessionId() method and standardized on setting acpClient.lastActiveSessionId = undefined.

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

Labels

agent:needs-work nc-review found blocking findings area:vscode VS Code extension and host integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Restarting ACP creates a new backend session without resetting or re-syncing the webview transcript

1 participant