Skip to content

feat(vscode): add real-time ACP status bar indicator and recovery actions - #1381

Merged
akramcodez merged 3 commits into
Nano-Collective:mainfrom
akramcodez:feat/vscode-acp-status-bar
Sep 18, 2026
Merged

akramcodez merged 3 commits into
Nano-Collective:mainfrom
akramcodez:feat/vscode-acp-status-bar

Conversation

@akramcodez

Copy link
Copy Markdown
Member

Description

Enhances the VS Code extension's status bar item to reflect the real-time lifecycle states of the ACP (Agent Client Protocol) process and adds recovery actions:

  • Real-Time ACP Status Display:
    • Added AcpStatusBarController and pure mapper describeAcpStatus to reflect granular agent states:
      • Starting: $(sync~spin) Nanocoder: Starting
      • Reconnecting: $(sync~spin) Nanocoder: Reconnecting (attempt/total)
      • Connected: $(check) Nanocoder (dynamically displays the active model name once synced via onStateSync)
      • Failed: $(error) Nanocoder: Failed with failure context and tooltip
      • CliMissing: $(plug) Nanocoder: Not installed
      • VersionMismatch: $(warning) Nanocoder: Update CLI
      • Disconnected: $(circle-slash) Nanocoder: Disconnected
  • Recovery Actions & Commands:
    • Registered nanocoder.restartAcp and nanocoder.showOutput commands.
    • Displays a recovery dialog when process retries are exhausted offering direct "Show Logs" and "Restart" actions.
  • State Lifecycle & Resilience:
    • Extended AcpStateManager with AcpStatusDetail to track retry counts and error reasons.
    • Decoupled AcpStateManager disposal from AcpProcessManager.dispose() so the shared singleton and its listeners persist across manual restarts.
  • Unit Testing:
    • Added test suites in plugins/vscode/src/acp-status-bar.spec.ts and plugins/vscode/src/acp-state.spec.ts covering status transitions, labels, tooltips, and click commands.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested status bar transitions during CLI startup, reconnect backoff, and crash failure scenarios in VS Code extension host.

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

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

Copy link
Copy Markdown
Contributor

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

@akramcodez — there is a blocking item below.

Adds a real-time ACP status bar controller with seven connection states (Starting, Reconnecting, Failed, CliMissing, VersionMismatch, Connected, Disconnected), one-click recovery actions, and decouples AcpStateManager disposal from AcpProcessManager so manual restarts don't break subscribers. Implementation is largely correct, the tests meaningfully exercise both the pure mapper and the controller's dialog-stacking guard, and the description-to-text mapping covers all states. There are two issues that should be fixed before merge: the changeset is filed against the wrong package (and the VS Code extension's own package is explicitly ignored by the workspace's changesets config), and the legacy updateStatusBar helper still overwrites the controller's text/tooltip/command whenever ACP is Connected/CliMissing/VersionMismatch.

🔴 blocking · changeset · .changeset/vscode-acp-status-bar.md

The changeset declares "@nanocollective/nanocoder": minor, but this PR only changes plugins/vscode/**, whose package is nanocoder-vscode. .changeset/config.json puts nanocoder-vscode in ignore, so the VS Code extension is deliberately excluded from the changelog pipeline. The right move is to delete the changeset file entirely — adding one against the CLI package will produce a nanocoder minor-release entry that misattributes VS Code-only work, and the rubric flags exactly this kind of wrong-package changeset as something that breaks release-prepare on the next push to main.

🟠 important · correctness · plugins/vscode/src/extension.ts:244

updateStatusBar's new agentBusy guard returns only for Starting | Restarting | Failed, so when the ACP process is in Connected (or CliMissing / VersionMismatch) the legacy companion's connect() success path still calls updateStatusBar(true), overwriting the controller's tooltip with 'Connected to Nanocoder CLI (companion)' and replacing command = 'nanocoder.restartAcp' with 'nanocoder.disconnect'. The new code's own comment says the legacy update should apply only while the agent is not healthy, but Connected is the healthy state and is unprotected — clicking the bar during a successful ACP session then disconnects the companion instead of restarting the CLI. Extend agentBusy to include Connected (and ideally the other controller-owned states), or have the controller re-assert itself after any external write.

🟠 important · completeness · plugins/vscode/src/extension.ts:269

The legacy case 'status' handler guards statusBarItem.text = ... behind acpStateManager.status !== ACPStatus.Connected, which is the right idea, but the companion is also responsible for setting tooltip and command via updateStatusBar. Because the Connected-vs-companion conflict described above isn't fully fixed, the bar's tooltip and click command can disagree with the controller's view. The same logic that blocks the status payload should apply to updateStatusBar.

⚪ nit · scope · plugins/vscode/src/extension.ts:179

config.get<boolean>('autoConnect', true) is changed to config.get<boolean>('autoConnect', false) to align with the package.json default. That's defensible on its own, but it's a user-visible behavior change (extension will no longer auto-dial the legacy companion by default) bundled into a feature PR about the ACP status bar. Worth calling out in the PR description or splitting into a separate commit so the change is reviewable on its own merits.


🔴 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 Sep 18, 2026
@akramcodez

Copy link
Copy Markdown
Member Author

/re-review

@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 1 important, 1 nit

@akramcodez — a few things worth a look, none blocking.

The previous review's blocking changeset finding is resolved (the wrong-package changeset has been removed), the agentBusy guard in updateStatusBar is now broader (!== Disconnected covers all controller-owned states), and the case 'status' handler has been tightened to match. Implementation is largely correct and the test suite exercises the new mapper, controller transitions, model sync, and dialog stacking meaningfully. One new correctness issue remains: when retries are exhausted the process manager sets Failed and calls vscode.window.showErrorMessage directly, while the controller simultaneously fires its own showErrorMessage recovery dialog on the same transition — two stacked dialogs in production.

Addressed since the last review

  • ✅ Blocking changeset finding: the wrong-package changeset has been removed (no .changeset/vscode-acp-status-bar.md exists, and nanocoder-vscode is in ignore), so the next release-prepare will not misattribute this VS Code-only work to a nanocoder minor release.
  • ✅ Important: updateStatusBar's agentActive guard now uses acpStateManager.status !== ACPStatus.Disconnected, which excludes every state the controller owns (Starting, Connected, Restarting, Failed, CliMissing, VersionMismatch) — the legacy companion can no longer clobber the controller's text/tooltip/command on a successful ACP session.
  • ✅ Important: the case 'status' handler in handleServerMessage now gates on acpStateManager.status === ACPStatus.Disconnected, so the companion model-text update can no longer disagree with the controller's Connected view. The same guard covers the legacy updateStatusBar writes as well, so the controller's text/tooltip/command are not overridden.
  • ✅ Nit: the previous nit about autoConnect being flipped from true to false in the same PR is moot — extension.ts:159 is unchanged in this diff (config.get<boolean>('autoConnect', true)).

🟠 important · correctness · plugins/vscode/src/acp-process-manager.ts:196

Two error dialogs fire when retries are exhausted. The new handleCrash exhaustion branch both sets ACPStatus.Failed (which the controller's onDidChangeStatus listener picks up and routes through showRecoveryDialog, opening a vscode.window.showErrorMessage with Show Logs / Restart choices) and calls vscode.window.showErrorMessage(...) itself. The dialogOpen guard only protects against multiple Failed events, not against the process manager also calling showErrorMessage on the same transition. The user sees two stacked error dialogs. Either drop the direct showErrorMessage call here and let the controller own the user-visible recovery flow, or have the controller skip its dialog when the process manager has just shown one.

⚪ nit · correctness · plugins/vscode/src/acp-state.ts

setStatus(newStatus, detail) guards the fire on this._status !== newStatus || detail !== undefined. So setStatus(Restarting, undefined) after setStatus(Restarting, {attempt: 2, totalAttempts: 5}) is a no-op and leaves the previous detail stuck on the state manager. The process manager always passes the detail pair together today so this never triggers in practice, but the contract is fragile: any future caller that clears detail by passing undefined will silently keep the stale {attempt, totalAttempts}. Either fire whenever detail differs from the stored one, or document that callers must pass the new value.


🔴 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 agent:comments nc-review left non-blocking findings and removed agent:needs-work nc-review found blocking findings labels Sep 18, 2026
@akramcodez
akramcodez merged commit 274385a into Nano-Collective:main Sep 18, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings area:vscode VS Code extension and host integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant