From b6a39dee7c11864d5aae70afb80257b78aa050e8 Mon Sep 17 00:00:00 2001 From: mmprotest Date: Mon, 29 Jun 2026 11:41:26 +1000 Subject: [PATCH] Fix Pi approval UI duplication --- integrations/pi-villani/README.md | 6 +- integrations/pi-villani/src/extension.test.ts | 76 ++++--------------- integrations/pi-villani/src/index.ts | 65 +++------------- integrations/pi-villani/src/render.test.ts | 21 +++-- integrations/pi-villani/src/render.ts | 4 +- 5 files changed, 47 insertions(+), 125 deletions(-) diff --git a/integrations/pi-villani/README.md b/integrations/pi-villani/README.md index eff11073..57794a51 100644 --- a/integrations/pi-villani/README.md +++ b/integrations/pi-villani/README.md @@ -3,10 +3,10 @@ Install with: ```bash pi install npm:@mmprotest/pi-villani ``` -Provides `/villani `, `/villani-abort`, and `/villani-confirm-test`. +Provides `/villani `. Runtime version: `v0.1.3`. -## Abort semantics +## Usage -When `/villani-abort` is invoked, the extension first aborts the Pi model proxy signal and sends a bridge `{ "type": "abort" }` command for the active run. Pending Villani approval requests are denied by the bridge immediately so approved writes do not continue after abort. The extension waits a short grace period for `run_aborted`; if it is not observed, the bridge subprocess is killed. Active child commands launched by the runtime may continue until that subprocess is killed, so abort guarantees no orphan bridge process rather than instant termination of every child process. +Run Villani with `/villani `. Active runs are still cleaned up automatically on process/session cancellation or error cleanup. diff --git a/integrations/pi-villani/src/extension.test.ts b/integrations/pi-villani/src/extension.test.ts index 43f7a4e5..e9a474e3 100644 --- a/integrations/pi-villani/src/extension.test.ts +++ b/integrations/pi-villani/src/extension.test.ts @@ -11,72 +11,19 @@ function api() { }, }; } -test("registers Villani Pi commands", () => { +test("registers only the public /villani command", () => { const a = api(); activate(a); - assert.deepEqual(Object.keys(a.commands), [ - "villani", + assert.deepEqual(Object.keys(a.commands), ["villani"]); + for (const removed of [ "villani-abort", "villani-confirm-test", "villani-ping", "villani-doctor", "villani-proxy-test", "villani-bridge-ping", - ]); -}); -test("/villani-ping only notifies OK", async () => { - const a = api(); - activate(a); - const notes: any[] = []; - await a.commands["villani-ping"].handler("", { - ui: { notify: (m: string) => notes.push(m) }, - }); - assert.deepEqual(notes, ["Villani ping OK"]); -}); -test("/villani-confirm-test accepted, rejected, and missing UI are visible", async () => { - for (const value of [true, false]) { - const a = api(); - activate(a); - const notes: string[] = []; - await a.commands["villani-confirm-test"].handler("", { - ui: { notify: (m: string) => notes.push(m), confirm: async () => value }, - }); - assert.equal(notes[0], "Villani confirmation test starting..."); - assert.equal( - notes.at(-1), - value ? "Villani confirmation accepted" : "Villani confirmation rejected", - ); - } - const a = api(); - activate(a); - const notes: string[] = []; - await a.commands["villani-confirm-test"].handler("", { - ui: { notify: (m: string) => notes.push(m) }, - }); - assert.deepEqual(notes, [ - "Villani confirmation test starting...", - "Villani confirmation UI unavailable", - ]); -}); -test("/villani-doctor prints diagnostics without secrets", async () => { - const a = api(); - activate(a); - const notes: string[] = []; - process.env.VILLANI_API_KEY = "secret"; - try { - await a.commands["villani-doctor"].handler("", { - cwd: "/tmp/repo", - model: { id: "m" }, - modelRegistry: { getApiKeyAndHeaders() {} }, - ui: { notify: (m: string) => notes.push(m) }, - }); - const msg = notes.join("\n"); - assert.match(msg, /package version: 0.1.4/); - assert.match(msg, /runtime version: 0.1.3/); - assert.match(msg, /ctx.model exists: true/); - assert.doesNotMatch(msg, /secret|VILLANI_API_KEY/); - } finally { - delete process.env.VILLANI_API_KEY; + ]) { + assert.equal(a.commands[removed], undefined); } }); import { @@ -447,7 +394,7 @@ test("approvalMessage never renders object and includes Bash command", async () input: { command: "echo hi" }, }); assert.doesNotMatch(msg, /\[object Object\]/); - assert.match(msg, /Command:\necho hi/); + assert.match(msg, /Command: echo hi/); assert.equal( approvalTitle({ tool: "Bash" }), "Villani requests command authority", @@ -475,7 +422,7 @@ test("confirm passes signal option to ctx.ui.confirm", async () => { assert.equal(args[2].signal, signal); }); -test("/villani approval pending sets status/widget and accepted clears widget", async () => { +test("/villani approval pending clears widget, sets status, confirms, and accepted clears widget", async () => { const old = process.env.VILLANI_COMMAND; const oldUsePi = process.env.VILLANI_USE_PI_MODEL; const p = bridgeScript( @@ -514,8 +461,13 @@ test("/villani approval pending sets status/widget and accepted clears widget", assert.ok( statuses.some((a) => a[0] === "villani" && /approval|authorization|authority|clearance/i.test(String(a[1]))), ); - assert.ok(widgets.some((a) => a[0] === "villani" && Array.isArray(a[1]) && a[1][0] === "Villaniclearance required")); - assert.doesNotMatch(JSON.stringify(widgets), /Pending approval|Allow this operation|\[object Object\]/); + assert.ok(widgets.length > 0); + assert.equal(widgets[0][0], "villani"); + assert.equal(widgets[0][1], undefined); + assert.equal(confirms[0][0], "Villani requests command authority"); + assert.match(confirms[0][1], /Command: echo hi/); + assert.match(confirms[0][1], /Approve this Villani action\?/); + assert.doesNotMatch(JSON.stringify(widgets), /Villani requests command authority|Command: echo hi|Pending approval|Allow this operation|\[object Object\]/); assert.ok(widgets.some((a) => a[0] === "villani" && a[1] === undefined)); assert.equal(confirms[0][2].signal instanceof AbortSignal, true); }); diff --git a/integrations/pi-villani/src/index.ts b/integrations/pi-villani/src/index.ts index 05efef5e..f1acaf6e 100644 --- a/integrations/pi-villani/src/index.ts +++ b/integrations/pi-villani/src/index.ts @@ -89,15 +89,6 @@ export function approvalTitle(request: any) { if (tool === "GitDiff") return "Villani requests diff inspection"; return "Villani requests approval"; } -function approvalLead(tool: string): string { - if (tool === "Bash") return "Villani requests command authority."; - if (tool === "Read") return "Villani requests dossier access."; - if (tool === "Write") return "Villani requests edit authority."; - if (tool === "Patch") return "Villani requests patch authority."; - if (tool === "GitStatus") return "Villani requests repository inspection."; - if (tool === "GitDiff") return "Villani requests diff inspection."; - return "Villaniclearance required."; -} export function approvalMessage(request: any) { const input = request.input && @@ -114,12 +105,15 @@ export function approvalMessage(request: any) { typeof request.path === "string" ? request.path : undefined; - const lines = [approvalLead(tool), ""]; - - lines.push("Operation:", tool, ""); + const lines: string[] = []; - if (command) lines.push("Command:", command, ""); - if (path) lines.push("File:", path, ""); + if (tool === "Bash" && command) lines.push(`Command: ${command}`, ""); + else if ((tool === "Write" || tool === "Patch") && path) lines.push(`File: ${path}`, ""); + else { + lines.push("Operation:", tool, ""); + if (command) lines.push(`Command: ${command}`, ""); + if (path) lines.push(`File: ${path}`, ""); + } lines.push("Approve this Villani action?"); return lines.join("\n"); @@ -132,8 +126,8 @@ async function handleApproval(run: ActiveRun, ctx: any, e: any) { let approved = false; const message = approvalMessage(e); try { - await setStatus(ctx, nextVillaniStatus("approval", requestId) ?? "Villani pauses for approval..."); - await setWidget(ctx, ["Villaniclearance required", message]); + await setWidget(ctx, undefined); + await setStatus(ctx, nextVillaniStatus("approval", requestId) ?? "Villaniclearance required..."); approved = await confirm(ctx, approvalTitle(e), message, { signal: run.abort.signal, }); @@ -149,7 +143,7 @@ async function handleApproval(run: ActiveRun, ctx: any, e: any) { } if (run.pending.get(requestId) !== false) return; run.pending.set(requestId, true); - await setStatus(ctx, nextVillaniStatus("thinking", "approval-resolved") ?? "Villanithoughts classified..."); + await setStatus(ctx, approved ? "Villani resumes operation..." : "Villani records denial..."); run.bridge?.respondToApproval(run.id, requestId, approved); if (process.env.VILLANI_PI_DEBUG === "1") console.error("[pi-villani bridge] approval response sent"); @@ -261,7 +255,7 @@ export async function runVillani( if (activeRun) { await notify( ctx, - "Villani is already running. Use /villani-abort first.", + "Villani is already running. Wait for the active run to finish or cancel the session.", "warn", ); return; @@ -620,41 +614,6 @@ export default function activate(api: any) { reg("villani", "Run Villani Code on a task", async (args, ctx) => safeCommand(ctx, "Villani", () => runVillani(args, ctx?.pi ?? api, ctx)), ); - reg("villani-abort", "Abort the active Villani run", async (_args, ctx) => - safeCommand(ctx, "Villani abort", async () => { - await abortVillani(ctx); - }), - ); - reg("villani-confirm-test", "Test Villani approval UI", async (_args, ctx) => - safeCommand(ctx, "Villani confirmation test", async () => { - await notify(ctx, "Villani confirmation test starting..."); - if (!ctx?.ui?.confirm) { - await notify(ctx, "Villani confirmation UI unavailable", "warn"); - return; - } - const ok = await confirm( - ctx, - "Villani confirmation test", - "Confirm Villani test?", - ); - await notify( - ctx, - ok ? "Villani confirmation accepted" : "Villani confirmation rejected", - ); - }), - ); - reg("villani-ping", "Check Villani extension loading", async (_args, ctx) => - safeCommand(ctx, "Villani ping", () => notify(ctx, "Villani ping OK")), - ); - reg("villani-doctor", "Show Villani diagnostics", async (_args, ctx) => - safeCommand(ctx, "Villani doctor", () => doctor(ctx)), - ); - reg("villani-proxy-test", "Test Villani Pi model proxy", async (_args, ctx) => - safeCommand(ctx, "Villani proxy test", () => proxyTest(ctx)), - ); - reg("villani-bridge-ping", "Ping Villani bridge stdio", async (_args, ctx) => - safeCommand(ctx, "Villani bridge ping", () => bridgePing(ctx)), - ); try { api.onSessionStart?.(async (ctx: any) => notify(ctx, "Villani extension loaded"), diff --git a/integrations/pi-villani/src/render.test.ts b/integrations/pi-villani/src/render.test.ts index 8eac3f10..f25c1030 100644 --- a/integrations/pi-villani/src/render.test.ts +++ b/integrations/pi-villani/src/render.test.ts @@ -96,8 +96,8 @@ test('approval message remains literal and readable', () => { const request = { tool: 'Bash', summary: 'Run command', input: { command: 'python -m pytest -v' } }; assert.equal(approvalTitle(request), 'Villani requests command authority'); const msg = approvalMessage(request); - assert.match(msg, /Villani requests command authority\./); - assert.match(msg, /Command:\npython -m pytest -v/); + assert.doesNotMatch(msg, /Villani requests command authority/); + assert.match(msg, /Command: python -m pytest -v/); assert.match(msg, /Approve this Villani action\?/); assert.doesNotMatch(msg, /Allow this operation\?/); assert.doesNotMatch(msg, /\[object Object\]/); @@ -124,8 +124,8 @@ test('approval_required uses approval category copy and widget avoids pending ap const rec = ctxRecorder(); await renderBridgeEvent({ type: 'approval_required', request_id: 'r1', tool: 'Read', summary: 'Read' }, {}, rec.ctx); assert.equal(rec.statuses.at(-1), 'Villani requires authorization...'); - assert.doesNotMatch(JSON.stringify(rec.widgets), /Pending approval/); - assert.match(JSON.stringify(rec.widgets), /Villaniclearance required/); + assert.deepEqual(rec.widgets, [undefined]); + assert.doesNotMatch(JSON.stringify(rec.widgets), /Pending approval|Villaniclearance required|Allow this operation|\[object Object\]/); }); test('approval titles use Villani-style copy for Read and Bash', () => { @@ -135,12 +135,23 @@ test('approval titles use Villani-style copy for Read and Bash', () => { test('approval message for Read without path is readable', () => { const msg = approvalMessage({ tool: 'Read', input: {} }); - assert.match(msg, /Villani requests dossier access\./); + assert.doesNotMatch(msg, /Villani requests dossier access/); assert.match(msg, /Operation:\nRead/); assert.match(msg, /Approve this Villani action\?/); assert.doesNotMatch(msg, /unknown|\[object Object\]|Allow this operation\?/i); }); + +test('approval_resolved accepted and denied clear widget with short continuation status', async () => { + for (const [approved, expected] of [[true, 'Villani resumes operation...'], [false, 'Villani records denial...']] as const) { + resetVillaniUiState(); + const rec = ctxRecorder(); + await renderBridgeEvent({ type: 'approval_resolved', approved }, {}, rec.ctx); + assert.equal(rec.statuses.at(-1), expected); + assert.deepEqual(rec.widgets, [undefined]); + } +}); + test('after approval_resolved status does not hardcode old thinking copy', () => { resetVillaniCopyCounters(); const state = reduceVillaniUiState({ phase: 'approval' }, { type: 'approval_resolved' }); diff --git a/integrations/pi-villani/src/render.ts b/integrations/pi-villani/src/render.ts index 2c662915..3036fb51 100644 --- a/integrations/pi-villani/src/render.ts +++ b/integrations/pi-villani/src/render.ts @@ -370,7 +370,7 @@ export function reduceVillaniUiState( } else if (event.type === "proxy_request_completed") { // Keep existing status; proxy completion is heartbeat-like UI noise. } else if (event.type === "approval_required") applyStatus("approval", event.request_id || event.requestId); - else if (event.type === "approval_resolved") applyStatus("thinking", "approval-resolved"); + else if (event.type === "approval_resolved") next.phase = event.approved === false ? "Villani records denial..." : "Villani resumes operation..."; else if (event.type === "tool_started") { const tool = String(event.tool || event.name || ""); const category = categoryForEvent(event) || "thinking"; @@ -464,8 +464,8 @@ export async function renderBridgeEvent( } if (event.type === "approval_required") { state = reduceVillaniUiState(state, event); + await setWidget(ctx, undefined); await setStatus(ctx, state.phase); - await setWidget(ctx, ["Villaniclearance required", event.summary || event.message || ""]); return; } if (event.type === "approval_resolved") {