From 630423ae9e98bb77e2966dbfc60638fd23f01281 Mon Sep 17 00:00:00 2001 From: mmprotest Date: Mon, 29 Jun 2026 10:33:20 +1000 Subject: [PATCH] Fix Pi Villani UI status and approval copy --- integrations/pi-villani/src/extension.test.ts | 9 +- integrations/pi-villani/src/index.ts | 74 +++++++----- integrations/pi-villani/src/render.test.ts | 63 ++++++++++- integrations/pi-villani/src/render.ts | 105 ++++++++++++++---- 4 files changed, 197 insertions(+), 54 deletions(-) diff --git a/integrations/pi-villani/src/extension.test.ts b/integrations/pi-villani/src/extension.test.ts index afaf822e..43f7a4e5 100644 --- a/integrations/pi-villani/src/extension.test.ts +++ b/integrations/pi-villani/src/extension.test.ts @@ -450,7 +450,7 @@ test("approvalMessage never renders object and includes Bash command", async () assert.match(msg, /Command:\necho hi/); assert.equal( approvalTitle({ tool: "Bash" }), - "Villani wants to run a shell command", + "Villani requests command authority", ); }); @@ -512,9 +512,10 @@ test("/villani approval pending sets status/widget and accepted clears widget", else process.env.VILLANI_USE_PI_MODEL = oldUsePi; } assert.ok( - statuses.some((a) => a[0] === "villani" && a[1] === "Waiting for approval..."), + 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]))); + 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.some((a) => a[0] === "villani" && a[1] === undefined)); assert.equal(confirms[0][2].signal instanceof AbortSignal, true); }); @@ -600,7 +601,7 @@ test("repeated model_request_started updates status without notify spam", async }; await renderBridgeEvent({ type: "model_request_started" }, {}, ctx); await renderBridgeEvent({ type: "model_request_started" }, {}, ctx); - assert.equal(statuses.length, 2); + assert.equal(statuses.length, 1); assert.ok(statuses.every((s) => /^Villani/.test(s))); assert.deepEqual(notes, []); }); diff --git a/integrations/pi-villani/src/index.ts b/integrations/pi-villani/src/index.ts index 6c018370..05efef5e 100644 --- a/integrations/pi-villani/src/index.ts +++ b/integrations/pi-villani/src/index.ts @@ -14,6 +14,7 @@ import { import { confirm, finalMessage, + nextVillaniStatus, notify, renderBridgeEvent, resetVillaniUiState, @@ -79,10 +80,23 @@ export async function safeCommand( } } export function approvalTitle(request: any) { - if (request.tool === "Bash") return "Villani wants to run a shell command"; - if (request.tool === "Write") return "Villani wants to write a file"; - if (request.tool === "Patch") return "Villani wants to apply a patch"; - return "Villani wants approval"; + const tool = String(request.tool || ""); + 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 "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 = @@ -91,16 +105,23 @@ export function approvalMessage(request: any) { !Array.isArray(request.input) ? request.input : {}; - const path = typeof input.path === "string" ? input.path : undefined; + + const tool = String(request.tool || "operation"); const command = typeof input.command === "string" ? input.command : undefined; - const safeSummary = - typeof request.summary === "string" && request.summary.trim() - ? request.summary - : "Approval required"; - const lines = [safeSummary, ""]; + const path = + typeof input.path === "string" ? input.path : + typeof input.file_path === "string" ? input.file_path : + typeof request.path === "string" ? request.path : + undefined; + + const lines = [approvalLead(tool), ""]; + + lines.push("Operation:", tool, ""); + if (command) lines.push("Command:", command, ""); - else if (path) lines.push("File:", path, ""); - lines.push("Allow this operation?"); + if (path) lines.push("File:", path, ""); + + lines.push("Approve this Villani action?"); return lines.join("\n"); } async function handleApproval(run: ActiveRun, ctx: any, e: any) { @@ -111,8 +132,8 @@ async function handleApproval(run: ActiveRun, ctx: any, e: any) { let approved = false; const message = approvalMessage(e); try { - await setStatus(ctx, "Waiting for approval..."); - await setWidget(ctx, ["Pending approval", message]); + await setStatus(ctx, nextVillaniStatus("approval", requestId) ?? "Villani pauses for approval..."); + await setWidget(ctx, ["Villaniclearance required", message]); approved = await confirm(ctx, approvalTitle(e), message, { signal: run.abort.signal, }); @@ -128,7 +149,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, "Villani is thinking..."); + await setStatus(ctx, nextVillaniStatus("thinking", "approval-resolved") ?? "Villanithoughts classified..."); run.bridge?.respondToApproval(run.id, requestId, approved); if (process.env.VILLANI_PI_DEBUG === "1") console.error("[pi-villani bridge] approval response sent"); @@ -232,7 +253,7 @@ export async function runVillani( ): Promise { resetVillaniUiState(); await notify(ctx, "Villani starting...", "info"); - await setStatus(ctx, "Starting Villani..."); + await setStatus(ctx, nextVillaniStatus("thinking", "startup") ?? "Villaniplan forming..."); if (!task?.trim()) { await notify(ctx, "/villani requires a task argument", "warn"); return; @@ -271,10 +292,10 @@ export async function runVillani( let config: any; let env: any; if (usePi) { - await setStatus(ctx, "Preparing model connection..."); + await setStatus(ctx, nextVillaniStatus("analysis", "model-connection") ?? "Villanalysis begins..."); model = await resolvePiModel(ctx); const auth = await resolveModelAuth(ctx, model); - await setStatus(ctx, "Preparing model connection..."); + await setStatus(ctx, nextVillaniStatus("analysis", "model-connection") ?? "Villanalysis begins..."); proxy = await startModelProxyFromPiModel({ model, apiKey: auth.apiKey, @@ -296,13 +317,13 @@ export async function runVillani( config = envConfig(); env = process.env; } - await setStatus(ctx, "Starting Villani..."); + await setStatus(ctx, nextVillaniStatus("thinking", "runtime-start") ?? "Villaniplan forming..."); const executable = await resolveVillaniRuntime({ signal: abort.signal }); if (process.env.VILLANI_COMMAND) { - await setStatus(ctx, "Starting Villani..."); + await setStatus(ctx, nextVillaniStatus("thinking", "runtime-start") ?? "Villaniplan forming..."); await assertBridgePing(executable, ctx, env, abort.signal); } - await setStatus(ctx, "Starting Villani..."); + await setStatus(ctx, nextVillaniStatus("thinking", "runtime-start") ?? "Villaniplan forming..."); const bridge = new VillaniBridgeProcess(executable, { env, startupTimeoutMs: 30000, @@ -311,16 +332,19 @@ export async function runVillani( cwd: ctx.cwd ?? process.cwd(), }); run.bridge = bridge; - await setStatus(ctx, "Starting Villani..."); + await setStatus(ctx, nextVillaniStatus("thinking", "runtime-start") ?? "Villaniplan forming..."); await bridge.waitUntilReady(undefined, abort.signal); - await setStatus(ctx, "Villani is thinking..."); + await setStatus(ctx, nextVillaniStatus("thinking", "approval-resolved") ?? "Villanithoughts classified..."); const startPostToolTimer = (eventType: string) => { clearPostToolTimer(); postToolTimer = setTimeout(async () => { try { bridge.send({ type: "ping", id: `${runId}-post-tool-ping` }); const pong = await bridge.waitForEvent("pong", 3000, abort.signal); - if (pong) await setStatus(ctx, "Villani is thinking..."); + if (pong) { + const status = nextVillaniStatus("thinking", "post-tool-ping"); + if (status) await setStatus(ctx, status); + } else await notify( ctx, @@ -419,7 +443,7 @@ export async function runVillani( mode: "runner", config, }); - await setStatus(ctx, "Villani is thinking..."); + await setStatus(ctx, nextVillaniStatus("thinking", "approval-resolved") ?? "Villanithoughts classified..."); try { await runStartedPromise; } catch (e) { diff --git a/integrations/pi-villani/src/render.test.ts b/integrations/pi-villani/src/render.test.ts index dcc2a640..8eac3f10 100644 --- a/integrations/pi-villani/src/render.test.ts +++ b/integrations/pi-villani/src/render.test.ts @@ -4,6 +4,7 @@ import { cleanAssistantText, reduceVillaniUiState, renderBridgeEvent, + nextVillaniStatus, resetVillaniCopyCounters, resetVillaniUiState, toolStartedMessage, @@ -93,8 +94,68 @@ test('cleanAssistantText trims without Villani prefix', () => { 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 wants to run a shell command'); + 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.match(msg, /Approve this Villani action\?/); + assert.doesNotMatch(msg, /Allow this operation\?/); assert.doesNotMatch(msg, /\[object Object\]/); }); + + +test('repeated model_request_started only updates status once within 12 seconds', () => { + resetVillaniCopyCounters(); + const first = reduceVillaniUiState({ phase: 'x' }, { type: 'model_request_started' }); + const second = reduceVillaniUiState(first, { type: 'model_request_started' }); + assert.equal(first.phase, 'Villani is make plan...'); + assert.equal(second.phase, first.phase); +}); + +test('proxy_request_started does not spam new thinking copy when already thinking', () => { + resetVillaniCopyCounters(); + const first = reduceVillaniUiState({ phase: 'x' }, { type: 'model_request_started' }); + const second = reduceVillaniUiState(first, { type: 'proxy_request_started' }); + assert.equal(second.phase, first.phase); +}); + +test('approval_required uses approval category copy and widget avoids pending approval', async () => { + resetVillaniCopyCounters(); + 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/); +}); + +test('approval titles use Villani-style copy for Read and Bash', () => { + assert.equal(approvalTitle({ tool: 'Read' }), 'Villani requests dossier access'); + assert.equal(approvalTitle({ tool: 'Bash' }), 'Villani requests command authority'); +}); + +test('approval message for Read without path is readable', () => { + const msg = approvalMessage({ tool: 'Read', input: {} }); + assert.match(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('after approval_resolved status does not hardcode old thinking copy', () => { + resetVillaniCopyCounters(); + const state = reduceVillaniUiState({ phase: 'approval' }, { type: 'approval_resolved' }); + assert.notEqual(state.phase, 'Villani is thinking...'); +}); + +test('villaniCopy approval rotates deterministically', () => { + resetVillaniCopyCounters(); + assert.equal(villaniCopy('approval'), 'Villani requires authorization...'); + assert.equal(villaniCopy('approval'), 'Villaniclearance required...'); +}); + +test('nextVillaniStatus suppresses repeated status until detail changes', () => { + resetVillaniCopyCounters(); + assert.equal(nextVillaniStatus('thinking', 'same'), 'Villani is make plan...'); + assert.equal(nextVillaniStatus('thinking', 'same'), undefined); + assert.equal(nextVillaniStatus('thinking', 'different'), 'Villaniplan forming...'); +}); diff --git a/integrations/pi-villani/src/render.ts b/integrations/pi-villani/src/render.ts index 12dc543c..ec6a96a6 100644 --- a/integrations/pi-villani/src/render.ts +++ b/integrations/pi-villani/src/render.ts @@ -7,6 +7,7 @@ export type VillaniCopyCategory = | "testing" | "debugging" | "review" + | "approval" | "failure" | "complete"; @@ -19,11 +20,26 @@ const VILLANI_COPY: Record = { testing: ["Villani begins inspection...", "Villanitest begins...", "Villani demands green tests...", "Villaniverdict pending...", "Villani checks for lies..."], debugging: ["Villani hunts weak bug...", "Villanidebug begins...", "Villani asks bug hard questions...", "Villanistack confesses...", "Villani removes instability..."], review: ["Villanireview begins...", "Villani judges patch...", "Villanicompliance checked...", "Villani approves, reluctantly...", "Villani checks for betrayal..."], + approval: [ + "Villani requires authorization...", + "Villaniclearance required...", + "Villani requests permission. Briefly.", + "Villani pauses for approval...", + "Villani awaits command authority...", + "Villani demands signed order...", + "Villanipermission pending...", + "Villani asks council. Reluctantly.", + ], failure: ["Villani sees failure. Unacceptable.", "Villanifailure recorded...", "Villani prepares punishment...", "Villani blames weak implementation...", "Villani demands second attempt..."], complete: ["Villanified. Accept result.", "Villani declares victory...", "Villani restores order...", "Villanivictory logged...", "Villani permits ship..."], }; const copyCounters = new Map(); +let lastStatusCategory: VillaniCopyCategory | undefined; +let lastStatusText: string | undefined; +let lastStatusAt = 0; +let lastStatusDetailKey: string | undefined; +let lastRenderedStatus: string | undefined; export function villaniCopy(category: VillaniCopyCategory): string { const options = VILLANI_COPY[category]; @@ -34,6 +50,37 @@ export function villaniCopy(category: VillaniCopyCategory): string { export function resetVillaniCopyCounters(): void { copyCounters.clear(); + resetVillaniStatusManager(); +} + +export function resetVillaniStatusManager(): void { + lastStatusCategory = undefined; + lastStatusText = undefined; + lastStatusAt = 0; + lastStatusDetailKey = undefined; + lastRenderedStatus = undefined; +} + +function shouldUpdateStatus(category: VillaniCopyCategory, detailKey?: string): boolean { + const now = Date.now(); + if (category !== lastStatusCategory) return true; + if (detailKey && detailKey !== lastStatusDetailKey) return true; + if (now - lastStatusAt > 12000) return true; + return false; +} + +export function nextVillaniStatus(category: VillaniCopyCategory, detailKey?: string): string | undefined { + if (!shouldUpdateStatus(category, detailKey)) return undefined; + const text = villaniCopy(category); + lastStatusCategory = category; + lastStatusText = text; + lastStatusAt = Date.now(); + lastStatusDetailKey = detailKey; + return text; +} + +export function currentStatusFallback(category: VillaniCopyCategory): string { + return lastStatusCategory === category && lastStatusText ? lastStatusText : villaniCopy(category); } export type VillaniUiState = { @@ -217,6 +264,7 @@ function categoryForEvent(event: any): VillaniCopyCategory | undefined { const phase = String(event.phase || ""); const tool = String(event.tool || event.name || ""); if (type === "model_request_started" || type === "proxy_request_started") return "thinking"; + if (type === "approval_required") return "approval"; if (type === "phase" && /diagnosis|planning/i.test(phase)) return "analysis"; if (type === "tool_started") { if (["Read", "GitStatus", "GitDiff", "GitLog"].includes(tool)) return "reading"; @@ -303,24 +351,28 @@ export function reduceVillaniUiState( event: any, ): VillaniUiState { const next = { ...state, lastEventAt: Date.now() }; - if (event.type === "run_started") next.phase = villaniCopy("thinking"); + const applyStatus = (category: VillaniCopyCategory, detailKey?: string) => { + const status = nextVillaniStatus(category, detailKey); + if (status) next.phase = status; + }; + if (event.type === "run_started") applyStatus("thinking", "run-started"); else if (event.type === "model_request_started") { - next.phase = villaniCopy("thinking"); + applyStatus("thinking", "model-request"); next.lastCommand = undefined; next.lastCommandExitCode = undefined; next.lastCommandPreview = undefined; } else if (event.type === "proxy_request_started") - next.phase = villaniCopy("thinking"); - else if (event.type === "model_request_completed") - next.phase = villaniCopy("thinking"); - else if (event.type === "proxy_request_completed") - next.phase = villaniCopy("thinking"); - else if (event.type === "approval_required") next.phase = "Waiting for approval..."; - else if (event.type === "approval_resolved") next.phase = villaniCopy("thinking"); + applyStatus("thinking", "model-request"); + else if (event.type === "model_request_completed") { + if (!state.phase) applyStatus("review", "model-completed"); + } 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 === "tool_started") { const tool = String(event.tool || event.name || ""); const category = categoryForEvent(event) || "thinking"; - next.phase = villaniCopy(category); + applyStatus(category, `${tool}:${pathFromEvent(event) || commandFromEvent(event) || ""}`); next.lastToolPath = pathFromEvent(event); next.lastToolKind = ["Read", "GitStatus", "GitDiff", "GitLog"].includes(tool) ? "reading" : (["Write", "Patch", "Edit"].includes(tool) ? "writing" : undefined); if (tool === "Bash") { @@ -330,12 +382,13 @@ export function reduceVillaniUiState( } } else if (event.type === "command_started") { - next.phase = villaniCopy(commandCategory(event.command)); - next.lastCommand = String(event.command || "").slice(0, 500); + const command = String(event.command || "").slice(0, 500); + applyStatus(commandCategory(event.command), command); + next.lastCommand = command; next.lastCommandExitCode = undefined; next.lastCommandPreview = undefined; } else if (event.type === "command_finished") { - next.phase = villaniCopy(Number(event.exit_code ?? 0) !== 0 ? "debugging" : "review"); + applyStatus(Number(event.exit_code ?? 0) !== 0 ? "debugging" : "review", String(event.command || next.lastCommand || "")); next.lastCommand = String(event.command || next.lastCommand || "").slice( 0, 500, @@ -343,27 +396,27 @@ export function reduceVillaniUiState( next.lastCommandExitCode = event.exit_code; next.lastCommandPreview = preview(event); } else if (event.type === "phase") { - next.phase = villaniCopy(categoryForEvent(event) || "thinking"); + applyStatus(categoryForEvent(event) || "thinking", String(event.phase || "")); } else if (event.type === "verification_started" || event.type === "validation_started") - next.phase = villaniCopy("testing"); + applyStatus("testing", event.type); else if (event.type === "verification_finished" || event.type === "validation_finished") - next.phase = villaniCopy("review"); + applyStatus("review", event.type); else if (event.type === "run_completed") { - next.phase = villaniCopy("complete"); + applyStatus("complete", "run-completed"); next.finalSummary = event.summary; next.changedFiles = visibleChangedFiles( event.changed_files || event.changedFiles || next.changedFiles || [], ); next.transcriptPath = event.transcript_path || event.transcriptPath; } else if (event.type === "run_failed") { - next.phase = villaniCopy("failure"); + applyStatus("failure", "run-failed"); next.finalSummary = event.summary || event.error; - } else if (event.type === "run_aborted") next.phase = villaniCopy("failure"); + } else if (event.type === "run_aborted") applyStatus("failure", "run-aborted"); else if ( event.type === "runner_heartbeat" && Date.now() - (state.lastEventAt || 0) > 15000 ) - next.phase = villaniCopy("thinking"); + applyStatus("thinking", "heartbeat"); return next; } export function widgetForState(state: VillaniUiState): any { @@ -383,10 +436,13 @@ export function widgetForState(state: VillaniUiState): any { return undefined; } export async function renderState(ctx: any, state: VillaniUiState) { - await setStatus(ctx, state.phase); + if (state.phase && state.phase !== lastRenderedStatus) { + await setStatus(ctx, state.phase); + lastRenderedStatus = state.phase; + } await setWidget(ctx, widgetForState(state)); } -let state: VillaniUiState = { phase: "Starting Villani..." }; +let state: VillaniUiState = { phase: "" }; export async function renderBridgeEvent( event: any, _pi: any, @@ -407,7 +463,7 @@ export async function renderBridgeEvent( if (event.type === "approval_required") { state = reduceVillaniUiState(state, event); await setStatus(ctx, state.phase); - await setWidget(ctx, ["Pending approval", event.summary || event.message || ""]); + await setWidget(ctx, ["Villaniclearance required", event.summary || event.message || ""]); return; } if (event.type === "approval_resolved") { @@ -490,5 +546,6 @@ export async function renderBridgeEvent( await renderState(ctx, state); } export function resetVillaniUiState() { - state = { phase: "Starting Villani..." }; + state = { phase: "" }; + resetVillaniStatusManager(); }