-
Notifications
You must be signed in to change notification settings - Fork 0
Interactive run transport — pillar 1 (copilot ACP) #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1281fe4
feat(rpc): server-request answering, notification subscription, outbo…
5988c3f
feat(events): tool/permission canonical events + pure ACP mappers fro…
567a7ba
feat(acp): persistent AcpSession + SessionManager behind the Transpor…
26d151d
fix(acp): bound runToChat lifetime + force disposal on app quit
abf56f3
feat(runtime): interactive copilot routing, permission IPC, tool/perm…
bf7d962
fix(store): tool/permission reducers target the last assistant turn (…
00b0a28
feat(ui): permission cards, expandable tool rows, stop button
908c11c
fix(acp/ui): expire permissions before terminal events; never rehydra…
77747cf
fix(acp): expand ~ in session cwd (copilot session/new rejects non-ab…
9cb92a0
docs: interactive run transport pillar 1 done — copilot ACP verified …
ea2d75b
fix(review): session-load throws to preserve context, dead-child reco…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import { describe, it, expect } from 'vitest' | ||
| import { homedir } from 'os' | ||
| import { pickAutoApprove, acpCwd } from './acpSession' | ||
|
|
||
| describe('pickAutoApprove', () => { | ||
| it('picks the first allow-kind option', () => { | ||
| expect(pickAutoApprove([ | ||
| { id: 'reject_once', label: 'Deny', kind: 'deny' }, | ||
| { id: 'allow_once', label: 'Allow once', kind: 'allow' }, | ||
| { id: 'allow_always', label: 'Always', kind: 'allow_always' } | ||
| ])?.id).toBe('allow_once') | ||
| }) | ||
| it('returns undefined when no allow option exists', () => { | ||
| expect(pickAutoApprove([{ id: 'reject_once', label: 'Deny', kind: 'deny' }])).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe('acpCwd', () => { | ||
| it('expands a stored ~ workspace path to absolute (copilot session/new rejects non-absolute)', () => { | ||
| expect(acpCwd('~/Code/nac-code')).toBe(`${homedir()}/Code/nac-code`) | ||
| expect(acpCwd('~')).toBe(homedir()) | ||
| }) | ||
| it('passes an absolute path through and falls back to process cwd when unset', () => { | ||
| expect(acpCwd('/abs/path')).toBe('/abs/path') | ||
| expect(acpCwd(undefined)).toBe(process.cwd()) | ||
| expect(acpCwd('')).toBe(process.cwd()) | ||
| }) | ||
| }) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| import { JsonRpcClient } from '../capabilities/jsonRpc' | ||
| import type { AgentEvent, PermissionOption } from '../../../shared/runtime' | ||
| import { mapAcpUpdate, mapPermissionRequest } from './mapAcp' | ||
| import { resolveCwd } from '../paths' | ||
|
|
||
| export const PROMPT_TIMEOUT_MS = 1_800_000 // 30 min — cancellation, not timeout, is the stop lever | ||
| const HANDSHAKE_TIMEOUT_MS = 10_000 | ||
|
|
||
| export interface TransportSession { | ||
| prompt(runId: string, text: string): void | ||
| respondPermission(requestId: string, optionId: string): void | ||
| cancel(): void | ||
| dispose(): void | ||
| } | ||
|
|
||
| /** Pure + exported for testing: YOLO auto-approval picks the first allow-ish option. */ | ||
| export function pickAutoApprove(options: PermissionOption[]): PermissionOption | undefined { | ||
| return options.find((o) => o.kind === 'allow' || o.kind === 'allow_always') | ||
| } | ||
|
|
||
| /** Pure + exported for testing: ACP session cwd. copilot's session/new rejects a non-absolute path | ||
| * (`-32603 "Directory path must be absolute"`), so a stored `~/…` workspace path MUST be expanded — | ||
| * the same resolveCwd every one-shot adapter uses. Falls back to process cwd when unset. */ | ||
| export function acpCwd(cwd: string | undefined): string { | ||
| return resolveCwd(cwd) ?? process.cwd() | ||
| } | ||
|
|
||
| interface PendingPermission { | ||
| resolve: (optionId: string) => void | ||
| denyId: string | ||
| } | ||
|
|
||
| export class AcpSession implements TransportSession { | ||
| private client: JsonRpcClient | ||
| private sessionId: string | null = null | ||
| private currentRunId: string | null = null | ||
| private replaying = false // suppress session/load history replay | ||
| private permissionSeq = 0 | ||
| private pendingPermissions = new Map<string, PendingPermission>() | ||
| private onEvent: (e: AgentEvent) => void | ||
| private yolo: boolean | ||
|
|
||
| constructor(onEvent: (e: AgentEvent) => void, yolo: boolean) { | ||
| this.onEvent = onEvent | ||
| this.yolo = yolo | ||
| this.client = new JsonRpcClient('copilot', ['--acp']) | ||
| this.client.onNotification('session/update', (params) => { | ||
| if (this.replaying || !this.currentRunId) return | ||
| const update = (params as { update?: unknown } | null)?.update | ||
| for (const e of mapAcpUpdate(this.currentRunId, update)) this.onEvent(e) | ||
| }) | ||
| this.client.onRequest('session/request_permission', (params) => this.handlePermission(params)) | ||
| } | ||
|
|
||
| setYolo(y: boolean): void { | ||
| this.yolo = y | ||
| } | ||
|
|
||
| /** Resolves the ACP handshake; throws on failure so the caller can fall back. */ | ||
| async connect(cwd: string | undefined, existingSessionId: string | undefined): Promise<string> { | ||
| await this.client.request('initialize', { | ||
| protocolVersion: 1, | ||
| clientCapabilities: { fs: { readTextFile: false, writeTextFile: false } } | ||
| }, HANDSHAKE_TIMEOUT_MS) | ||
| if (existingSessionId) { | ||
| try { | ||
| this.replaying = true // session/load re-emits history as session/update — never re-append it | ||
| await this.client.request('session/load', { sessionId: existingSessionId, cwd: acpCwd(cwd), mcpServers: [] }, HANDSHAKE_TIMEOUT_MS) | ||
| this.sessionId = existingSessionId | ||
| return existingSessionId | ||
| } catch (e) { | ||
| // Re-throw: the caller sent a BARE message (renderer chose native continuity, no replay | ||
| // text seeded). Falling through to session/new here would silently start an empty | ||
| // session and drop the conversation — a hard context-preservation violation. Rejecting | ||
| // connect() instead makes promptViaAcp resolve { ok: false }, so ipc.ts falls back to the | ||
| // one-shot startCopilotRun(sessionId) path, which uses --resume to preserve context. | ||
| throw e instanceof Error ? e : new Error(String(e)) | ||
| } finally { | ||
| this.replaying = false | ||
| } | ||
| } | ||
| const res = (await this.client.request('session/new', { cwd: acpCwd(cwd), mcpServers: [] }, HANDSHAKE_TIMEOUT_MS)) as { sessionId?: string } | ||
| if (!res?.sessionId) throw new Error('acp: session/new returned no sessionId') | ||
| this.sessionId = res.sessionId | ||
| return res.sessionId | ||
| } | ||
|
|
||
| get loadedSessionId(): string | null { | ||
| return this.sessionId | ||
| } | ||
|
|
||
| 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 } }) | ||
| } | ||
| this.onEvent(event) | ||
| const denyId = event.options.find((o) => o.kind === 'deny')?.id ?? event.options[event.options.length - 1].id | ||
| return new Promise((resolve) => { | ||
| this.pendingPermissions.set(requestId, { | ||
| denyId, | ||
| resolve: (optionId) => { | ||
| this.onEvent({ type: 'permission.resolved', runId, requestId, optionId }) | ||
| resolve({ outcome: { outcome: 'selected', optionId } }) | ||
| } | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| /** True while a turn is in flight — the idle reaper must never dispose a busy session. */ | ||
| get busy(): boolean { | ||
| return this.currentRunId !== null | ||
| } | ||
|
|
||
| prompt(runId: string, text: string): void { | ||
| if (!this.sessionId) throw new Error('acp: no session') | ||
| this.currentRunId = runId | ||
| this.onEvent({ type: 'run.started', runId, sessionId: this.sessionId }) | ||
| this.client | ||
| .request('session/prompt', { sessionId: this.sessionId, prompt: [{ type: 'text', text }] }, PROMPT_TIMEOUT_MS) | ||
| .then((res) => { | ||
| const stop = (res as { stopReason?: string } | null)?.stopReason | ||
| this.expirePermissions() // resolve open cards BEFORE the terminal event unmaps the run (Critical 1: order matters) | ||
| this.onEvent({ type: 'run.completed', runId, stopReason: stop === 'cancelled' ? 'canceled' : 'end_turn' }) | ||
| }) | ||
| .catch((e: Error) => { | ||
| this.expirePermissions() | ||
| this.onEvent({ type: 'run.errored', runId, message: e.message }) | ||
| }) | ||
| .finally(() => { | ||
| this.currentRunId = null | ||
| }) | ||
| } | ||
|
|
||
| respondPermission(requestId: string, optionId: string): void { | ||
| const p = this.pendingPermissions.get(requestId) | ||
| if (!p) return | ||
| this.pendingPermissions.delete(requestId) | ||
| p.resolve(optionId) | ||
| } | ||
|
|
||
| private expirePermissions(): void { | ||
| // A turn ended with cards still open (error/cancel): answer the protocol with a deny-equivalent | ||
| // — the actual deny option id offered for that request, not a hardcoded guess. | ||
| for (const [requestId, p] of this.pendingPermissions) { | ||
| this.pendingPermissions.delete(requestId) | ||
| p.resolve(p.denyId) | ||
| } | ||
| } | ||
|
|
||
| /** True once the underlying ACP child process has exited — the session can no longer be used. */ | ||
| get dead(): boolean { | ||
| return this.client.isClosed | ||
| } | ||
|
|
||
| cancel(): void { | ||
| if (this.sessionId) this.client.notify('session/cancel', { sessionId: this.sessionId }) | ||
| } | ||
|
|
||
| dispose(): void { | ||
| this.expirePermissions() | ||
| this.client.close() | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { describe, it, expect } from 'vitest' | ||
| import { mapAcpUpdate, mapPermissionRequest } from './mapAcp' | ||
|
|
||
| const TOOL_CALL = { sessionUpdate: 'tool_call', toolCallId: 'call_MHx', title: 'Run echo nac-probe-ok', kind: 'execute', status: 'pending', rawInput: { command: 'echo nac-probe-ok', description: 'Run echo nac-probe-ok', mode: 'sync' } } | ||
| const TOOL_DONE = { sessionUpdate: 'tool_call_update', toolCallId: 'call_MHx', status: 'completed', content: [{ type: 'content', content: { type: 'text', text: 'nac-probe-ok\n<shellId: 0 completed with exit code 0>' } }], rawOutput: { content: 'nac-probe-ok\n<shellId: 0 completed with exit code 0>' } } | ||
| const CHUNK = { sessionUpdate: 'agent_message_chunk', content: { type: 'text', text: 'It printed ' } } | ||
| const PERM = { sessionId: 's1', toolCall: { toolCallId: 'call_MHx', title: 'Run echo nac-probe-ok', kind: 'execute', status: 'pending', rawInput: { command: 'echo nac-probe-ok' } }, options: [{ optionId: 'allow_once', kind: 'allow_once', name: 'Allow once' }, { optionId: 'allow_always', kind: 'allow_always', name: 'Always allow' }, { optionId: 'reject_once', kind: 'reject_once', name: 'Deny' }] } | ||
|
|
||
| describe('mapAcpUpdate', () => { | ||
| it('maps tool_call to a pending tool.updated carrying the command as detail', () => { | ||
| expect(mapAcpUpdate('r', TOOL_CALL)).toEqual([{ type: 'tool.updated', runId: 'r', toolCallId: 'call_MHx', title: 'Run echo nac-probe-ok', kind: 'execute', status: 'pending', detail: 'echo nac-probe-ok' }]) | ||
| }) | ||
| it('maps a completed tool_call_update carrying output text as detail', () => { | ||
| const [e] = mapAcpUpdate('r', TOOL_DONE) | ||
| expect(e).toMatchObject({ type: 'tool.updated', toolCallId: 'call_MHx', status: 'completed' }) | ||
| expect((e as { detail?: string }).detail).toContain('nac-probe-ok') | ||
| }) | ||
| it('maps agent_message_chunk to content.delta and ignores unknown/junk updates', () => { | ||
| expect(mapAcpUpdate('r', CHUNK)).toEqual([{ type: 'content.delta', runId: 'r', streamKind: 'assistant_text', text: 'It printed ' }]) | ||
| expect(mapAcpUpdate('r', { sessionUpdate: 'plan' })).toEqual([]) | ||
| expect(mapAcpUpdate('r', null)).toEqual([]) | ||
| }) | ||
| it('preserves a tool_call_update without status as a running upsert', () => { | ||
| const [e] = mapAcpUpdate('r', { sessionUpdate: 'tool_call_update', toolCallId: 'call_MHx', content: [{ type: 'content', content: { type: 'text', text: 'partial' } }] }) | ||
| expect(e).toMatchObject({ type: 'tool.updated', status: 'running', detail: 'partial' }) | ||
| }) | ||
| it('never surfaces a non-string rawOutput.content as detail (Minor 6: React would crash on an object child)', () => { | ||
| const [e] = mapAcpUpdate('r', { sessionUpdate: 'tool_call_update', toolCallId: 'call_MHx', status: 'completed', rawOutput: { content: { type: 'image', data: 'base64...' } } }) | ||
| expect(e).toMatchObject({ type: 'tool.updated', toolCallId: 'call_MHx', status: 'completed' }) | ||
| expect((e as { detail?: unknown }).detail).toBeUndefined() | ||
| }) | ||
| }) | ||
|
|
||
| describe('mapPermissionRequest', () => { | ||
| it('maps the captured request with normalized option kinds', () => { | ||
| const e = mapPermissionRequest('r', 'req1', PERM) | ||
| expect(e).toEqual({ | ||
| type: 'permission.requested', runId: 'r', requestId: 'req1', title: 'Run echo nac-probe-ok', | ||
| detail: 'echo nac-probe-ok', | ||
| options: [ | ||
| { id: 'allow_once', label: 'Allow once', kind: 'allow' }, | ||
| { id: 'allow_always', label: 'Always allow', kind: 'allow_always' }, | ||
| { id: 'reject_once', label: 'Deny', kind: 'deny' } | ||
| ] | ||
| }) | ||
| }) | ||
| it('returns null for junk', () => { | ||
| expect(mapPermissionRequest('r', 'x', null)).toBeNull() | ||
| expect(mapPermissionRequest('r', 'x', { options: [] })).toBeNull() | ||
| }) | ||
| }) |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in PR #4 — handlePermission now auto-cancels (resolves the JSON-RPC request with a cancelled outcome) when
!currentRunIdorreplaying, via a pure testedshouldAutoCancelPermissionguard that mirrors the existing session/update notification guard. No more unresolvable pending permission / deadlock, and replayed prompts during session/load no longer surface.