Interactive run transport — pillar 1 (copilot ACP) - #3
Merged
Conversation
…m captured frames Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Delete runToChat entries when terminal run events occur (run.completed/run.errored) - Add force flag to disposeChat to allow killing busy sessions on app quit - disposeAll now force-disposes all sessions (process is exiting) - Idle reaper path unchanged (re-arms instead of killing mid-turn) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ission turn state
…late-event race) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…te streaming; neutral resolved glyph Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…solute paths) Found during live verification: interactive copilot runs silently fell back to headless because AcpSession passed the stored workspace path (~/Code/...) straight to session/new, which returns -32603 'Directory path must be absolute'. Route it through resolveCwd like every one-shot adapter does.
…live (allow/deny/cancel/YOLO/continuity/switch) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…very, no ledger poisoning, live event sink, provider-switch dispose, react-safe detail Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds an interactive, persistent ACP-backed transport for the Copilot provider, allowing mid-run permission prompts, structured tool streaming, and protocol-level cancellation while preserving the “replay uses only turn text” invariant for cross-provider context portability.
Changes:
- Extend the canonical
AgentEventstream to include tool/permission lifecycle events and add an IPC channel for permission responses. - Implement a persistent per-chat ACP session manager (
copilot --acp) using an enhanced JSON-RPC client that supports server→client requests + notifications. - Update the renderer store + UI to render tool rows and permission cards inline in the transcript, and normalize persisted state to avoid “live-looking” rehydration.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/runtime.ts | Adds chatId, permission option types, new AgentEvent variants, and IPC channel for permission responses. |
| src/renderer/src/store/store.ts | Adds tool/permission state to turns and reducers to upsert/resolve them (targets last assistant turn). |
| src/renderer/src/store/store.test.ts | Tests tool upsert merging, permission resolution, and late-event targeting logic. |
| src/renderer/src/store/runtime.ts | Wires new AgentEvents into the store; forwards chatId; adds runId reverse lookup for Stop/permissions. |
| src/renderer/src/store/persist.ts | Normalizes persisted tool/permission state to prevent rehydrating “live” UI state. |
| src/renderer/src/store/persist.test.ts | Tests hydration normalization for streaming/tools/permissions. |
| src/renderer/src/components/ToolRow.tsx | New UI component to render expandable tool call rows. |
| src/renderer/src/components/PermissionCard.tsx | New UI component to render inline permission cards and resolved summaries. |
| src/renderer/src/components/ChatView.tsx | Renders tool rows + permission cards in message view; adds Stop button and permission response wiring. |
| src/preload/index.ts | Exposes runs.respondPermission over the typed IPC bridge. |
| src/main/runtime/ipc.ts | Routes Copilot runs through ACP first with fallback to headless; adds permission response + protocol cancel routing. |
| src/main/runtime/capabilities/jsonRpc.ts | Adds params, server-request/notification handling, request answering, and “closed” safety checks. |
| src/main/runtime/capabilities/jsonRpc.test.ts | Tests message classification for response vs server-request vs notification. |
| src/main/runtime/acp/sessionManager.ts | New per-chat ACP session lifecycle manager with idle reaping and run↔chat mapping. |
| src/main/runtime/acp/mapAcp.ts | New mappers from ACP update/permission frames into canonical AgentEvents. |
| src/main/runtime/acp/mapAcp.test.ts | Tests ACP frame mapping behavior and safety around non-string outputs. |
| src/main/runtime/acp/acpSession.ts | New ACP transport session implementation (connect/load/new, prompt, permission handling, cancel, dispose). |
| src/main/runtime/acp/acpSession.test.ts | Tests auto-approve selection and cwd resolution behavior. |
| docs/DECISIONS.md | Records the pillar-1 interactive Copilot ACP decision and verification notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+92
to
+100
| private handlePermission(params: unknown): Promise<unknown> { | ||
| const runId = this.currentRunId ?? 'unknown' | ||
| const requestId = `perm_${++this.permissionSeq}` | ||
| const event = mapPermissionRequest(runId, requestId, params) | ||
| if (!event) return Promise.resolve({ outcome: { outcome: 'cancelled' } }) // zero options: never hang | ||
| if (this.yolo) { | ||
| const auto = pickAutoApprove(event.options) | ||
| if (auto) return Promise.resolve({ outcome: { outcome: 'selected', optionId: auto.id } }) | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in PR #4 — handlePermission now auto-cancels (resolves the JSON-RPC request with a cancelled outcome) when !currentRunId or replaying, via a pure tested shouldAutoCancelPermission guard that mirrors the existing session/update notification guard. No more unresolvable pending permission / deadlock, and replayed prompts during session/load no longer surface.
Comment on lines
+79
to
+82
| if (!ok) { | ||
| handler({ type: 'content.delta', runId, streamKind: 'assistant_text', text: '\n[interactive session unavailable — ran headless]\n' }) | ||
| runs.set(runId, startCopilotRun(runId, { prompt: req.prompt, cwd: req.cwd, yolo: req.yolo, sessionId: req.sessionId, effort: req.effort, model: req.model }, handler)) | ||
| } |
Owner
Author
There was a problem hiding this comment.
Fixed in PR #4 — the fallback note is now a render-only tool.updated row (kind:'notice') instead of a content.delta, so it never enters turn.text and buildReplayPrompt stays clean across cross-provider switches.
njfife
added a commit
that referenced
this pull request
Jul 9, 2026
Address Copilot review on PR #3 (permission deadlock + replay cleanliness)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Copilot runs are now interactive instead of one-shot headless. A persistent
copilot --acpsession per chat sits behind a provider-genericTransportSessionseam, so the harness can ask for permission mid-run, tool activity streams as structure, and cancellation is real.[tool: X]text.session/cancel("cancelled by user"), not a process kill.session/load, falling back to--resumeif load fails.JsonRpcClientgained server→client request answering (how permission responses flow) + notification subscription; three new canonicalAgentEvents (tool.updated,permission.requested,permission.resolved) drive the UI. Pillars 2–4 (codex app-server, claude, opencode acp) reuse this seam.buildReplayPromptreads only turn text, so cross-provider switches stay bounded and clean.Verification
sleep→ "cancelled by user"; YOLO → no card; two-turn native continuity; copilot→Claude switch recalled all prior context (no tool chatter in replay); full app-restart revival recalled a planted codeword (context preserved across a fresh process).AcpSessionpassed the stored~/…workspace path tosession/new, which copilot rejects (-32603non-absolute) — every interactive run silently fell back to headless; fixed viaresolveCwd. (2) The final review found asession/load-failure path that silently dropped context (the project's hardest invariant) — fixed to throw→--resumeand live-verified via the restart-revival test.docs/, subagent-driven implementation with per-task adversarial reviews and a final whole-branch review (verdict: ready to merge). Riding follow-ups (model-over-ACP forwarding, provider-switch-while-busy edge) noted indocs/DECISIONS.md.🤖 Generated with Claude Code