Skip to content

Fix Pi approval UI duplication and remove public debug slash commands #416

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.

Sign up for GitHub

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
mmprotest merged 1 commit into from
Jun 29, 2026
Merged

Fix Pi approval UI duplication and remove public debug slash commands #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions integrations/pi-villani/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ Install with:
```bash
pi install npm:@mmprotest/pi-villani
```
Provides `/villani <task>`, `/villani-abort`, and `/villani-confirm-test`.
Provides `/villani <task>`.

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 <task>`. Active runs are still cleaned up automatically on process/session cancellation or error cleanup.
76 changes: 14 additions & 62 deletions integrations/pi-villani/src/extension.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
});
Expand Down
65 changes: 12 additions & 53 deletions integrations/pi-villani/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand All @@ -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");
Expand All @@ -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,
});
Expand All @@ -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");
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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"),
Expand Down
21 changes: 16 additions & 5 deletions integrations/pi-villani/src/render.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\]/);
Expand All @@ -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', () => {
Expand All @@ -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' });
Expand Down
4 changes: 2 additions & 2 deletions integrations/pi-villani/src/render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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") {
Expand Down
Loading