mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 11:01:16 +01:00
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 50600786f7 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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
07721e33f7
commit
8bd45b5b30
7 files changed
+167
-6
No files matched your search
@@ -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`
|
||||
|
||||
@@ -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: '<img src="x" onerror="alert(1)">', 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");
|
||||
|
||||
@@ -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 ? <ToolActionResolution state={state} interaction={interaction} /> : null}
|
||||
</div>
|
||||
</div>
|
||||
{isPending ? inspectableArguments ? (
|
||||
<details className="mt-3">
|
||||
<summary className="cursor-pointer text-xs font-medium text-muted-foreground">
|
||||
Arguments and audit hash
|
||||
</summary>
|
||||
<pre className="mt-2 max-h-40 overflow-auto whitespace-pre-wrap break-words font-mono text-xs text-foreground">
|
||||
{payload.argumentsSummaryJson}
|
||||
</pre>
|
||||
<p className="mt-1 break-all font-mono text-xs text-muted-foreground">{payload.argumentsHash}</p>
|
||||
</details>
|
||||
) : <p className="mt-3 text-xs text-muted-foreground">Request arguments are unavailable.</p> : null}
|
||||
{isPending ? (
|
||||
<DecisionCardFooter leading={footerSlot} className="mt-3">
|
||||
<div className={cn(!footerSlot && "mt-3", "flex flex-wrap items-center justify-end gap-2")}>
|
||||
|
||||
@@ -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> = {},
|
||||
): 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 <TaskChatInteractionCard item={interactionItem(interaction)}
|
||||
presentation={reviewing ? "takeover" : "timeline"}
|
||||
onReviewRequest={() => setReviewing(true)}
|
||||
onAcceptInteraction={accept} onRejectInteraction={reject} />;
|
||||
}
|
||||
flushSync(() => root.render(<TooltipProvider><ThemeProvider><ReviewSurface /></ThemeProvider></TooltipProvider>));
|
||||
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(
|
||||
|
||||
@@ -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: {
|
||||
|
||||
@@ -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<typeof import("@/context/CompanyContext")>(),
|
||||
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(<QueryClientProvider client={client}><TooltipProvider><ThemeProvider>
|
||||
<ReviewQueueCard emptyState="reassure" />
|
||||
</ThemeProvider></TooltipProvider></QueryClientProvider>));
|
||||
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 () => {
|
||||
|
||||
@@ -35,6 +35,7 @@ import {
|
||||
notCreatorRequestConfirmationInteraction,
|
||||
pendingToolActionDestructiveInteraction,
|
||||
pendingToolActionWriteInteraction,
|
||||
pendingToolActionLongProposalInteraction,
|
||||
runningToolActionInteraction,
|
||||
issueThreadInteractionComments,
|
||||
issueThreadInteractionEvents,
|
||||
@@ -918,7 +919,7 @@ export const ToolActionPendingWrite: Story = {
|
||||
<StoryFrame>
|
||||
<ScenarioCard
|
||||
title="Pending · write"
|
||||
description="A write tool call awaits approval: identity header, WRITE risk badge, humanized preview, collapsible technical details, expiry countdown, and an Approve & run CTA."
|
||||
description="A write tool call awaits approval: app identity, a short preview, optional redacted arguments and audit hash, and Approve & run or Decline."
|
||||
>
|
||||
<ToolActionCard
|
||||
interaction={pendingToolActionWriteInteraction}
|
||||
@@ -929,6 +930,19 @@ export const ToolActionPendingWrite: Story = {
|
||||
),
|
||||
};
|
||||
|
||||
export const ToolActionLongProposal: Story = {
|
||||
render: () => (
|
||||
<StoryFrame>
|
||||
<ScenarioCard
|
||||
title="Pending · proposal beyond the short preview"
|
||||
description="Open Arguments and audit hash to inspect the server-redacted proposal, including its final launch date, before approving. Inspection does not submit a decision."
|
||||
>
|
||||
<ToolActionCard interaction={pendingToolActionLongProposalInteraction} interactive />
|
||||
</ScenarioCard>
|
||||
</StoryFrame>
|
||||
),
|
||||
};
|
||||
|
||||
export const ToolActionPendingDestructive: Story = {
|
||||
render: () => (
|
||||
<StoryFrame>
|
||||
|
||||
Reference in new issue
Block a user