From a4c10dd3a1ca17eb2f539e1eb58f76e582084d46 Mon Sep 17 00:00:00 2001 From: Aei <256851514+aeitwoen@users.noreply.github.com> Date: Mon, 31 Aug 2026 12:10:20 +0200 Subject: [PATCH 1/2] feat(pi): gate pull request creation --- packages/pi/extensions/forges.ts | 23 +++++- test/extensions.test.ts | 121 +++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 1 deletion(-) diff --git a/packages/pi/extensions/forges.ts b/packages/pi/extensions/forges.ts index 2417002..788adf3 100644 --- a/packages/pi/extensions/forges.ts +++ b/packages/pi/extensions/forges.ts @@ -37,6 +37,10 @@ function loadToolOperations(): Promise { return toolOperationsPromise; } +function pullRequestApprovalMessage(params: ForgesTools.CreatePullRequestParams): string { + return `This will create a hosted pull request with the following payload:\n\n${JSON.stringify(params, null, 2)}`; +} + export default function forgesExtension(pi: ExtensionAPI): void { pi.registerTool({ name: "forges_repos_list", @@ -298,7 +302,24 @@ export default function forgesExtension(pi: ExtensionAPI): void { "Use forges_pull_requests_create only when the user explicitly asks to create a pull request.", ], parameters: createPullRequestParameters, - async execute(_toolCallId, params) { + async execute(_toolCallId, params, signal, _onUpdate, ctx) { + if (!ctx.hasUI) { + throw new Error( + "Pull request creation requires interactive approval in Pi TUI or RPC mode", + ); + } + + const approved = await ctx.ui.confirm( + "Create pull request?", + pullRequestApprovalMessage(params), + { signal }, + ); + if (!approved) { + throw new Error( + "Pull request creation was cancelled by the user. Do not retry unless the user asks again.", + ); + } + return (await loadToolOperations()).createPullRequest(params); }, }); diff --git a/test/extensions.test.ts b/test/extensions.test.ts index b2b26e1..a1cd5b5 100644 --- a/test/extensions.test.ts +++ b/test/extensions.test.ts @@ -142,6 +142,13 @@ function ompAccepts(tool: OmpToolDefinition, value: unknown): boolean { const unusedPiContext = {} as PiExtensionContext; const unusedOmpContext = {} as OmpExtensionContext; +function approvalPiContext( + confirm: PiExtensionContext["ui"]["confirm"], + hasUI = true, +): PiExtensionContext { + return { hasUI, ui: { confirm } } as unknown as PiExtensionContext; +} + beforeEach(() => { vi.clearAllMocks(); resetPinnedProviders(); @@ -172,6 +179,22 @@ beforeEach(() => { createdAt: "2026-08-18T00:00:00Z", updatedAt: "2026-08-18T00:00:00Z", }); + mocks.pullRequests.create.mockResolvedValue({ + id: "43", + number: 43, + title: "Add approval", + body: "Require confirmation before creation.", + state: "open", + labels: [], + author: { login: "oritwoen" }, + assignees: [{ login: "reviewer" }], + createdAt: "2026-09-01T00:00:00Z", + updatedAt: "2026-09-01T00:00:00Z", + sourceBranch: "feat/pr-approval", + targetBranch: "main", + merged: false, + draft: true, + }); }); afterEach(() => { @@ -406,6 +429,104 @@ describe("Forges Pi extension", () => { expect(result.details.result).toEqual({ items: [], hasNextPage: false }); }); + it("fails closed when pull-request creation has no approval UI", async () => { + const confirm = vi.fn(); + const tool = requirePiTool(registerPiTools(), "forges_pull_requests_create"); + + await expect( + tool.execute( + "test", + { + platform: "github", + owner: "agntn", + repo: "forges", + title: "Add approval", + body: "Require confirmation before creation.", + sourceBranch: "feat/pr-approval", + targetBranch: "main", + }, + undefined, + undefined, + approvalPiContext(confirm, false), + ), + ).rejects.toThrow("requires interactive approval"); + + expect(confirm).not.toHaveBeenCalled(); + expect(mocks.pullRequests.create).not.toHaveBeenCalled(); + }); + + it("does not create a pull request when Pi approval is declined", async () => { + const confirm = vi.fn().mockResolvedValue(false); + const tool = requirePiTool(registerPiTools(), "forges_pull_requests_create"); + + await expect( + tool.execute( + "test", + { + platform: "github", + owner: "agntn", + repo: "forges", + title: "Add approval", + body: "Require confirmation before creation.", + sourceBranch: "feat/pr-approval", + targetBranch: "main", + draft: true, + assignees: ["reviewer"], + }, + undefined, + undefined, + approvalPiContext(confirm), + ), + ).rejects.toThrow("cancelled by the user"); + + expect(confirm).toHaveBeenCalledOnce(); + expect(confirm).toHaveBeenCalledWith( + "Create pull request?", + expect.stringContaining('"sourceBranch": "feat/pr-approval"'), + { signal: undefined }, + ); + expect(mocks.pullRequests.create).not.toHaveBeenCalled(); + }); + + it("creates a pull request after Pi approval", async () => { + const confirm = vi.fn().mockResolvedValue(true); + const tool = requirePiTool(registerPiTools(), "forges_pull_requests_create"); + const params = { + platform: "github" as const, + owner: "agntn", + repo: "forges", + title: "Add approval", + body: "Require confirmation before creation.", + sourceBranch: "feat/pr-approval", + targetBranch: "main", + draft: true, + assignees: ["reviewer"], + }; + + const result = await tool.execute( + "test", + params, + undefined, + undefined, + approvalPiContext(confirm), + ); + + expect(confirm).toHaveBeenCalledWith( + "Create pull request?", + expect.stringContaining(JSON.stringify(params, null, 2)), + { signal: undefined }, + ); + expect(mocks.pullRequests.create).toHaveBeenCalledWith("agntn", "forges", { + title: "Add approval", + body: "Require confirmation before creation.", + sourceBranch: "feat/pr-approval", + targetBranch: "main", + draft: true, + assignees: ["reviewer"], + }); + expect(result.details.result).toMatchObject({ number: 43 }); + }); + it("reloads authentication through the shared operation", async () => { const user = { id: "1", login: "aeitwoen" }; mocks.users.authenticated.mockResolvedValue(user); From 73b406f1c245583e7a408bef18901d943f4648b1 Mon Sep 17 00:00:00 2001 From: Aei <256851514+aeitwoen@users.noreply.github.com> Date: Mon, 31 Aug 2026 12:26:17 +0200 Subject: [PATCH 2/2] fix(pi): clarify pull request approval --- packages/pi/extensions/forges.ts | 42 +++++++++++++++++++++++++++- test/extensions.test.ts | 47 ++++++++++++++++++++++++++++++-- 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/packages/pi/extensions/forges.ts b/packages/pi/extensions/forges.ts index 788adf3..853e26b 100644 --- a/packages/pi/extensions/forges.ts +++ b/packages/pi/extensions/forges.ts @@ -1,5 +1,6 @@ import { existsSync } from "node:fs"; import { fileURLToPath } from "node:url"; +import { stripVTControlCharacters } from "node:util"; import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; @@ -37,8 +38,47 @@ function loadToolOperations(): Promise { return toolOperationsPromise; } +const platformLabels: Record = { + github: "GitHub", + gitlab: "GitLab", + gitea: "Gitea", +}; +// oxlint-disable-next-line eslint/no-control-regex -- Removing terminal control bytes is intentional. +const controlCharacter = /[\u0000-\u0009\u000B-\u001F\u007F-\u009F]/g; +const formatOrLineSeparator = /[\p{Cf}\p{Zl}\p{Zp}]/gu; +const loneSurrogate = /[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(? { expect(confirm).toHaveBeenCalledOnce(); expect(confirm).toHaveBeenCalledWith( "Create pull request?", - expect.stringContaining('"sourceBranch": "feat/pr-approval"'), + expect.stringContaining("Branches feat/pr-approval → main"), { signal: undefined }, ); expect(mocks.pullRequests.create).not.toHaveBeenCalled(); @@ -513,7 +513,18 @@ describe("Forges Pi extension", () => { expect(confirm).toHaveBeenCalledWith( "Create pull request?", - expect.stringContaining(JSON.stringify(params, null, 2)), + [ + "Repository agntn/forges on GitHub", + "Branches feat/pr-approval → main", + "Status Draft", + "Assignees reviewer", + "", + "Title", + "Add approval", + "", + "Description", + "Require confirmation before creation.", + ].join("\n"), { signal: undefined }, ); expect(mocks.pullRequests.create).toHaveBeenCalledWith("agntn", "forges", { @@ -527,6 +538,38 @@ describe("Forges Pi extension", () => { expect(result.details.result).toMatchObject({ number: 43 }); }); + it("sanitizes pull-request approval fields before rendering them", async () => { + const confirm = vi.fn().mockResolvedValue(false); + const tool = requirePiTool(registerPiTools(), "forges_pull_requests_create"); + + await expect( + tool.execute( + "test", + { + platform: "github", + owner: "agntn", + repo: "forges", + title: "Title\u001B]0;owned\u0007", + body: `Line one\u009B31mred\nLine two${String.fromCodePoint(0xd800)}end`, + sourceBranch: "feat/pr-approval", + targetBranch: "main", + }, + undefined, + undefined, + approvalPiContext(confirm), + ), + ).rejects.toThrow("cancelled by the user"); + + const message = confirm.mock.calls[0]?.[1]; + expect(message).toContain("Title\nTitle"); + expect(message).toContain("Description\nLine one red\nLine two end"); + expect(message).not.toMatch( + // oxlint-disable-next-line eslint/no-control-regex -- The assertion detects unsafe terminal bytes. + /[\u0000-\u0009\u000B-\u001F\u007F-\u009F\uD800-\uDFFF\u2028\u2029]/u, + ); + expect(mocks.pullRequests.create).not.toHaveBeenCalled(); + }); + it("reloads authentication through the shared operation", async () => { const user = { id: "1", login: "aeitwoen" }; mocks.users.authenticated.mockResolvedValue(user);