From 8bd45b5b30801c167b9e03f1938d6a0ff69bdd08 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 3 Nov 2026 06:28:21 -0600 Subject: [PATCH] fix(ui): expose redacted arguments in pending tool reviews (#18734) ## Thinking Path > - Paperclip helps people manage AI agents and review their actions. > - Ask first tool actions require a person to inspect the proposed operation. > - The task and Apps review surfaces share the pending tool-action card. > - That card shows a short preview but omits the available redacted argument body. > - A valid preview can omit important text, such as the date at the end of a proposal. > - This pull request adds optional argument inspection to the shared card. > - The person can inspect the proposed content before using the existing decision controls. ## Linked Issues or Issue Description Refs: #16540. Related PRs: #16571 exposes reviewed arguments on formal board approvals and scrolls the task review into view. #16716 routes tool actions to their required formal board approvals. This change addresses the separate shared pending tool-action card used by the task takeover and linked Apps reviews. It does not close the other reported paths in #16540. ## What Changed - Add the existing Arguments and audit hash disclosure pattern to the shared pending card. - Show only the bounded argument summary already redacted by the server and its audit hash. Show an unavailable message for missing, malformed, or oversized fields. - Keep the current approval, decline, and remembered-scope callbacks. - Add a long-proposal story and update the tool-review feature recipe. - Add React coverage for the real task review transition, the linked Apps review, plain text rendering, invalid fields, and unchanged decision calls. ## Verification - The new task-review test failed on the parent because the disclosure was absent. It opens Review request, inspects the proposal tail and hash, then approves the same interaction. - Oversized-whitespace controls fail on the previous guard ordering because trimming runs before the length bound. The guard now checks length first for both fields. - Passed: 117 React tests across TaskChatInteractionCard, IssueThreadInteractionCard, and ReviewQueueCard. The tests use the repository UI config with the existing installed Vitest 5.0.3 dependency tree. - Passed: token gates, map checks, Storybook catalog checks, and git diff checks. - Local full UI typecheck did not pass. The linked dependency tree lacks adapter and json-schema-to-ts declarations and has stale shared exports. It reports no diagnostics in the changed files. This is not a successful full typecheck. - Full workspace build, browser, and provider checks were not run locally. Current head 50600786f755b6602989f3b0f88cd090ff948196 passed all 48 normal CI jobs with one expected skip. Greptile and Paperclip Review both report 5/5 at this head; all review threads are resolved. No tool action or model call was run for this change. - Inspect the new ToolActionLongProposal story. Open Arguments and audit hash and read the final launch date. Inspection must not submit a decision. Existing destructive, running, failed, declined, and expired states keep their controls. ## Risks - This changes the pending review presentation in tasks and linked Apps reviews. - The UI displays the existing server-redacted projection. It does not request or reconstruct signed execution arguments. The existing server schema caps the summary at 20,000 characters and the hash at 255 characters. The UI uses the same limits and escapes both as text. - The server still owns redaction, scope checks, approval, and execution. Inspection makes no API mutation. The current callbacks retain the same request identity and permission scope. - The PR will remain unmerged for separate review and candidate-source integration. ## Model Used OpenAI `gpt-6.1-sol` (Codex), with source inspection, code editing, React tests, and Git/GitHub tools. The context window size is not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- maps/feature-map/questions-and-approvals.md | 4 +- .../IssueThreadInteractionCard.test.tsx | 48 ++++++++++++++++++- .../components/IssueThreadInteractionCard.tsx | 17 +++++++ .../TaskChatInteractionCard.test.tsx | 39 ++++++++++++++- .../issueThreadInteractionFixtures.ts | 10 ++++ ui/src/pages/apps/ReviewQueueCard.test.tsx | 39 ++++++++++++++- .../issue-thread-interactions.stories.tsx | 16 ++++++- 7 files changed, 167 insertions(+), 6 deletions(-) diff --git a/maps/feature-map/questions-and-approvals.md b/maps/feature-map/questions-and-approvals.md index 8c400bfd5c1..6f217864f50 100644 --- a/maps/feature-map/questions-and-approvals.md +++ b/maps/feature-map/questions-and-approvals.md @@ -28,7 +28,7 @@ Agent access: [companion](../agent-access-map/questions-and-approvals.md). resolution. A visible request is not necessarily actionable by that viewer. - `governance`: `questions-and-approvals/governance` — pending/history views, approve/reject, revision request, and resubmission for formal board approvals. -- `tool-review`: `questions-and-approvals/tool-review` — approve once, always allow the displayed scope, or decline; +- `tool-review`: `questions-and-approvals/tool-review` — inspect the bounded server-redacted arguments and audit hash before approving once, always allowing the displayed scope, or declining; decision status and provider execution outcome remain distinct. - `continuation`: `questions-and-approvals/continuation` — one recorded response leads to the eligible waiting run or continuation; repeated clicks, reloads, and competing tabs do not repeat work. @@ -98,7 +98,7 @@ Use these checks alongside the entry-point recipes below and the operation recor | `questions-and-approvals/plan-review` | Open the exact requested plan revision; reject one request and accept another. Inspect saved revision, work mode, owner, and the resulting continuation. | Edit after requesting review and reject stale acceptance. Test finalization readiness and bound completion review separately. In Agent Chat, inspect the later project/task creation and its source-plan provenance; acceptance alone is not a completed handoff. | | `questions-and-approvals/audience` | Repeat a request for a named person, named agent, human-only resolver, and independent reviewer. Inspect actionability and final attribution. | Test creator exclusion, wrong audience/company, private task, missing or cancelled run, and changed responsible-user access. A human comment used by an agent remains subject to that agent’s resolver authority. | | `questions-and-approvals/governance` | Create and inspect a formal request, comment, request revision, resubmit, then approve or reject distinct requests. Reload history and linked task state. | Test board decision authority separately from agent request/comment/resubmit authority. A repeated decision cannot repeat activation. Current pending status, payload revision, and governed side effects must agree. | -| `questions-and-approvals/tool-review` | Invoke a harmless Ask first action, compare task and Apps review surfaces, then exercise Approve once, Always allow, and Decline separately. | No provider call before approval. Repeat decisions and compare invocation count. Change the tool definition, revoke access, or set Off before a decision. Record the approval decision and provider outcome independently. | +| `questions-and-approvals/tool-review` | Invoke a harmless Ask first action with a proposal longer than its preview. Open Arguments and audit hash on task and Apps review surfaces and inspect the tail before a decision. Then exercise Approve once, Always allow, and Decline separately. | No provider call before approval. Repeat decisions and compare invocation count. Change the tool definition, revoke access, or set Off before a decision. Record the approval decision and provider outcome independently. | | `questions-and-approvals/continuation` | After answering, inspect the saved response, delivery receipt, eligible run, and useful result in the original task/thread. | Reload, race two tabs, and interrupt delivery after saving. Require one continuation without repeated external actions. Reassignment, Stop, pause, budget, and external-thread boundaries remain effective. | ### `task-thread` diff --git a/ui/src/components/IssueThreadInteractionCard.test.tsx b/ui/src/components/IssueThreadInteractionCard.test.tsx index fd1953ac26b..25f6849efe3 100644 --- a/ui/src/components/IssueThreadInteractionCard.test.tsx +++ b/ui/src/components/IssueThreadInteractionCard.test.tsx @@ -1263,7 +1263,7 @@ describe("IssueThreadInteractionCard", () => { }); describe("IssueThreadInteractionCard tool-action card", () => { - it("shows only the request description, app icon, and decision controls", () => { + it("keeps the request description and decisions primary with optional argument inspection", () => { const host = renderCard({ interaction: pendingToolActionWriteInteraction, onAcceptInteraction: vi.fn(), onRejectInteraction: vi.fn() }); expect(host.textContent).toContain("Add 1 row"); expect(host.querySelector('[role="img"]')?.getAttribute("aria-label")).toBe("Google Sheets"); @@ -1282,6 +1282,52 @@ describe("IssueThreadInteractionCard tool-action card", () => { expect(host.textContent).toContain("Launch checklist"); }); + it("renders only the server-redacted proposal as plain text when inspected", async () => { + const interaction = structuredClone(pendingToolActionWriteInteraction); + interaction.payload.toolAction!.argumentsSummaryJson = JSON.stringify({ text: '', apiKey: "[redacted]" }); + const accept = vi.fn(); + const host = renderCard({ interaction, onAcceptInteraction: accept }); + const details = host.querySelector("details")!; + expect(details.open).toBe(false); + await act(async () => details.querySelector("summary")!.click()); + expect(details.open).toBe(true); + expect(details.querySelector("pre")?.textContent).toBe(interaction.payload.toolAction!.argumentsSummaryJson); + expect(details.querySelector("img")).toBeNull(); + expect(details.textContent).toContain("[redacted]"); + expect(accept).not.toHaveBeenCalled(); + }); + + it.each([undefined, { secret: "must not render" }, "x".repeat(20_001)])("withholds missing, malformed or oversized proposal details (%#)", summary => { + const interaction = structuredClone(pendingToolActionWriteInteraction); + Object.assign(interaction.payload.toolAction!, { argumentsSummaryJson: summary }); + const host = renderCard({ interaction }); + expect(host.querySelector("details")).toBeNull(); + expect(host.textContent).toContain("Request arguments are unavailable."); + expect(host.textContent).not.toContain("must not render"); + expect(host.querySelector("pre")).toBeNull(); + expect(Array.from(host.querySelectorAll("button")).find(button => button.textContent === "Approve & run")?.disabled).toBe(true); + }); + + it.each(["argumentsSummaryJson", "argumentsHash"] as const)("rejects oversized whitespace before trimming %s", field => { + const interaction = structuredClone(pendingToolActionWriteInteraction); + const oversized = " ".repeat(field === "argumentsSummaryJson" ? 20_001 : 256); + interaction.payload.toolAction![field] = oversized; + const trim = String.prototype.trim; + let oversizedTrimCalls = 0; + const spy = vi.spyOn(String.prototype, "trim").mockImplementation(function (this: string) { + if (String(this) === oversized) oversizedTrimCalls++; + return trim.call(this); + }); + try { + const host = renderCard({ interaction }); + expect(host.querySelector("details")).toBeNull(); + expect(host.textContent).toContain("Request arguments are unavailable."); + expect(oversizedTrimCalls).toBe(0); + } finally { + spy.mockRestore(); + } + }); + it("keeps the destructive request warning and approval styling", () => { const host = renderCard({ interaction: pendingToolActionDestructiveInteraction, onAcceptInteraction: vi.fn(), onRejectInteraction: vi.fn() }); expect(host.textContent).toContain("cannot be undone"); diff --git a/ui/src/components/IssueThreadInteractionCard.tsx b/ui/src/components/IssueThreadInteractionCard.tsx index c3ba890e901..38281d787d1 100644 --- a/ui/src/components/IssueThreadInteractionCard.tsx +++ b/ui/src/components/IssueThreadInteractionCard.tsx @@ -1799,6 +1799,12 @@ function RequestToolActionCard({ const resolutionErrorMessage = useResolutionErrorMessage(); const isPending = state === "pending"; const variant = payload.risk === "destructive" ? "destructive" : "cta"; + // This server projection is already redacted and bounded by the interaction schema. + const inspectableArguments = typeof payload.argumentsSummaryJson === "string" + && payload.argumentsSummaryJson.length <= 20_000 && payload.argumentsSummaryJson.trim().length > 0 + && typeof payload.argumentsHash === "string" && payload.argumentsHash.length <= 255 + && payload.argumentsHash.trim().length > 0; + useEffect(() => { if (!isPending) setWorking(null); @@ -1828,6 +1834,17 @@ function RequestToolActionCard({ {!isPending ? : null} + {isPending ? inspectableArguments ? ( +
+ + Arguments and audit hash + + 
+            {payload.argumentsSummaryJson}
+
+

{payload.argumentsHash}

+
+ ) :

Request arguments are unavailable.

: null} {isPending ? (
diff --git a/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx b/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx index 2bd4ca44564..2640ae8a041 100644 --- a/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx +++ b/ui/src/components/task-chat/TaskChatInteractionCard.test.tsx @@ -1,6 +1,6 @@ // @vitest-environment jsdom -import { act, forwardRef, useImperativeHandle, type ForwardedRef } from "react"; +import { act, forwardRef, useImperativeHandle, useState, type ForwardedRef } from "react"; import { flushSync } from "react-dom"; import { createRoot, type Root } from "react-dom/client"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -13,6 +13,7 @@ import { ThemeProvider } from "@/context/ThemeContext"; import { TooltipProvider } from "@/components/ui/tooltip"; import { expiredSecretProposalInteraction, + pendingToolActionLongProposalInteraction, pendingAskUserQuestionsInteraction, pendingRequestCheckboxConfirmationInteraction, pendingRequestItemVerdictsInteraction, @@ -56,6 +57,8 @@ vi.mock("@/components/MarkdownEditor", () => ({ }), })); +(globalThis as typeof globalThis & { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + function createRequestConfirmation( overrides: Partial = {}, ): RequestConfirmationInteraction { @@ -118,6 +121,40 @@ describe("TaskChatInteractionCard", () => { container.remove(); }); + it("opens the real tool review and reveals the proposal tail before approval", async () => { + const accept = vi.fn(); + const reject = vi.fn(); + const interaction = structuredClone(pendingToolActionLongProposalInteraction); + function ReviewSurface() { + const [reviewing, setReviewing] = useState(false); + return setReviewing(true)} + onAcceptInteraction={accept} onRejectInteraction={reject} />; + } + flushSync(() => root.render()); + expect(container.textContent).not.toContain("November 8, 2030"); + await act(async () => { + Array.from(container.querySelectorAll("button")).find(button => button.textContent === "Review request")!.click(); + }); + const details = container.querySelector("details")!; + expect(details).not.toBeNull(); + expect(details.open).toBe(false); + expect(container.querySelector("pre")?.textContent).toBe(interaction.payload.toolAction!.argumentsSummaryJson); + expect(details.textContent).toContain("November 8, 2030"); + expect(details.textContent).toContain("[redacted]"); + expect(details.textContent).toContain(interaction.payload.toolAction!.argumentsHash); + await act(async () => details.querySelector("summary")!.click()); + expect(details.open).toBe(true); + expect(accept).not.toHaveBeenCalled(); + expect(reject).not.toHaveBeenCalled(); + await act(async () => { + Array.from(container.querySelectorAll("button")).find(button => button.textContent === "Approve & run")!.click(); + }); + expect(accept).toHaveBeenCalledExactlyOnceWith(interaction, undefined, undefined, false); + expect(reject).not.toHaveBeenCalled(); + }); + it("does not render a pending confirmation in the timeline", () => { flushSync(() => { root.render( diff --git a/ui/src/fixtures/issueThreadInteractionFixtures.ts b/ui/src/fixtures/issueThreadInteractionFixtures.ts index 8eea3363cb6..e4a49d57ab1 100644 --- a/ui/src/fixtures/issueThreadInteractionFixtures.ts +++ b/ui/src/fixtures/issueThreadInteractionFixtures.ts @@ -656,6 +656,16 @@ export const pendingToolActionWriteInteraction = createToolActionConfirmationInt id: "interaction-tool-action-pending-write", }); +// A legitimate humanized preview is shorter than the redacted argument body. +const longToolProposal = "The release note covers the agreed accessibility checklist, rollout owners, support contacts, and readiness review for the public launch. Launch date: November 8, 2030."; +export const pendingToolActionLongProposalInteraction = createToolActionConfirmationInteraction({ + id: "interaction-tool-action-long-proposal", + toolAction: { + previewMarkdown: `Add the public launch note.\n\n**Text:** ${longToolProposal.slice(0, 137)}…`, + argumentsSummaryJson: JSON.stringify({ text: longToolProposal, apiKey: "[redacted]" }, null, 2), + }, +}); + export const pendingToolActionDestructiveInteraction = createToolActionConfirmationInteraction({ id: "interaction-tool-action-pending-destructive", toolAction: { diff --git a/ui/src/pages/apps/ReviewQueueCard.test.tsx b/ui/src/pages/apps/ReviewQueueCard.test.tsx index e8171650057..aa3766e3d6b 100644 --- a/ui/src/pages/apps/ReviewQueueCard.test.tsx +++ b/ui/src/pages/apps/ReviewQueueCard.test.tsx @@ -5,8 +5,12 @@ import { createRoot } from "react-dom/client"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ReviewQueueCard } from "./ReviewQueueCard"; +import { pendingToolActionLongProposalInteraction } from "@/fixtures/issueThreadInteractionFixtures"; +import { ThemeProvider } from "@/context/ThemeContext"; +import { TooltipProvider } from "@/components/ui/tooltip"; const listActionRequestsMock = vi.hoisted(() => vi.fn()); +const listInteractionsMock = vi.hoisted(() => vi.fn()); const approveActionRequestMock = vi.hoisted(() => vi.fn()); const createTrustRuleFromActionRequestMock = vi.hoisted(() => vi.fn()); const pushToastMock = vi.hoisted(() => vi.fn()); @@ -21,7 +25,12 @@ vi.mock("@/api/tools", () => ({ }, })); -vi.mock("@/context/CompanyContext", () => ({ +vi.mock("@/api/issues", () => ({ + issuesApi: { listInteractions: (...args: unknown[]) => listInteractionsMock(...args) }, +})); + +vi.mock("@/context/CompanyContext", async importOriginal => ({ + ...await importOriginal(), useCompany: () => ({ selectedCompanyId: "company-1", selectedCompany: { id: "company-1", name: "Paperclip" }, @@ -123,6 +132,34 @@ describe("ReviewQueueCard", () => { await flushReact(); } + it("shows the linked interaction's redacted proposal tail in Apps before a decision", async () => { + const interaction = structuredClone(pendingToolActionLongProposalInteraction); + const item = pendingRequest(); + const linked = { ...item, request: { ...item.request, interactionId: interaction.id } }; + listActionRequestsMock.mockResolvedValue({ actionRequests: [linked] }); + listInteractionsMock.mockResolvedValue([interaction]); + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + root = createRoot(container); + await act(async () => root.render( + + )); + await flushReact(); + await vi.waitFor(() => expect(container.querySelector("details")).not.toBeNull()); + const details = container.querySelector("details")!; + expect(details).not.toBeNull(); + expect(details.open).toBe(false); + expect(details.textContent).toContain("November 8, 2030"); + expect(details.querySelector("pre")?.textContent).toBe(interaction.payload.toolAction!.argumentsSummaryJson); + await act(async () => details.querySelector("summary")!.click()); + expect(details.open).toBe(true); + expect(approveActionRequestMock).not.toHaveBeenCalled(); + expect(createTrustRuleFromActionRequestMock).not.toHaveBeenCalled(); + expect(listInteractionsMock).toHaveBeenCalledWith("issue-1"); + await act(async () => { buttonContaining("Approve & run")!.click(); }); + await vi.waitFor(() => expect(approveActionRequestMock).toHaveBeenCalledExactlyOnceWith("company-1", "request-1", false)); + expect(createTrustRuleFromActionRequestMock).not.toHaveBeenCalled(); + }); + it("submits approval and remembered permission as one atomic decision", async () => { const calls: string[] = []; approveActionRequestMock.mockImplementation(async () => { diff --git a/ui/storybook/stories/questions-and-approvals/issue-thread-interactions.stories.tsx b/ui/storybook/stories/questions-and-approvals/issue-thread-interactions.stories.tsx index 9a33db7e2ed..3e981571a8a 100644 --- a/ui/storybook/stories/questions-and-approvals/issue-thread-interactions.stories.tsx +++ b/ui/storybook/stories/questions-and-approvals/issue-thread-interactions.stories.tsx @@ -35,6 +35,7 @@ import { notCreatorRequestConfirmationInteraction, pendingToolActionDestructiveInteraction, pendingToolActionWriteInteraction, + pendingToolActionLongProposalInteraction, runningToolActionInteraction, issueThreadInteractionComments, issueThreadInteractionEvents, @@ -918,7 +919,7 @@ export const ToolActionPendingWrite: Story = { ( + + + + + + ), +}; + export const ToolActionPendingDestructive: Story = { render: () => (