mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 12:59:13 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents move their work into review, and a reviewer must then give a verdict on it > - By default anyone with write access can give that verdict, including the agent that did the work > - The server can constrain that default per issue with a `reviewPolicy` column, but no screen showed the value > - A reviewer could therefore press Approve on a review that the server refuses, and get a 403 > - This pull request shows the policy as a badge on the two surfaces where a person gives a verdict > - It also makes an agent verdict read as a verdict in the activity timeline > - The benefit is that a reviewer sees who can approve before they try ## Linked Issues or Issue Description No public GitHub issue exists for this change. The description below follows `.github/ISSUE_TEMPLATE/enhancement.yml`. **What existing behavior does this improve?** The issue review flow. A reviewer cannot see the approval constraint on an issue before they give a verdict. **Subsystem affected** Web UI (`ui/`), with one supporting change in the server attention service. **Current behavior** The server stores an optional approval constraint for each issue in a `reviewPolicy` column. The column has three meaningful states: the default (`NULL` or `anyone`), `not_creator`, and `human_only`. The server enforces the constraint when it receives a verdict. No screen shows the value. Two problems follow: 1. A reviewer presses Approve on a review that the server refuses. The server answers 403, and the reason is not visible on the card. 2. An agent that accepts or rejects a review renders in the activity timeline as the raw action id, for example "issue thread interaction accepted". A person who reads the timeline cannot tell that a verdict was given. **Proposed behavior** Show the constraint as a read-only badge on the two surfaces where a person gives a verdict. Show no pixels for the default state, because the default is what every issue already does. Make an agent verdict read as a verdict in the timeline. Only agents set the column today, so this change adds no control to set it. **Reason and benefit** A reviewer sees the constraint before they act. This prevents the 403, and it removes the need to explain the 403 afterwards. The timeline also becomes complete, because it now shows agent verdicts and human verdicts in the same way. **Breaking changes** None. The change adds a badge and changes copy. It adds no column, no endpoint, and no request. **Additional context** The server-side column and the verdict enforcement landed earlier in #10931. This pull request is the user interface for that column. The default state stays unchanged on screen, so the badge appears on a small number of issues. ## What Changed - **A read-only "Approvals" row** in the issue Execution properties. The row renders *only* for a constrained policy: "Anyone else" (`not_creator`) or "Human only" (`human_only`). A `NULL` or `anyone` column adds no row, so the panel is untouched on the overwhelming majority of issues. - **The same badge on the stalled-review card** in `/decisions`, above the three review verbs. A reviewer now sees the constraint before they press Approve. The condition is the same, so the default card is unchanged. - **Agent verdicts read as verdicts in the activity timeline.** An agent that accepted or rejected a review request previously rendered the raw action id ("issue thread interaction accepted"). It now reads "approved the request". A stalled-review decision names the verb that the actor chose. - **A cleared policy reports as "anyone", not "none",** in the field-change receipt. The `reviewPolicy` column is nullable by default, so an absent value is a real setting rather than a missing one. - **All copy comes from `ui/src/lib/review-policy.ts`.** Its badge lookup returns `null` for the default. This makes "no pixels for the default" one enforced decision instead of a condition repeated at each call site. It also keeps the badge, the activity line, and the receipt reading alike. - **The server attention service carries the policy** on the review attention subject, so the stalled-review card can read it. ## Verification Automated tests: - `ui/src/lib/review-policy.test.ts` — the default returns no badge, however the column spells it (`null`, `undefined`, `"anyone"`). An unrecognised policy from the wire shows nothing rather than leaking an enum value. - `ui/src/components/AttentionQueueRow.test.tsx` — no badge on the default card, and the verbs still render. Suppression of the badge must not suppress the card. - `ui/src/components/IssueProperties.test.tsx` — no Approvals row on the default. The constrained row contains no `button`, so nothing there can PATCH. - `server/src/__tests__/attention-service.test.ts` — the review attention subject carries the policy, and subjects built from narrower selects do not claim one. Run them with: ```sh pnpm vitest run ui/src/lib/review-policy.test.ts \ ui/src/components/AttentionQueueRow.test.tsx \ ui/src/components/IssueProperties.test.tsx \ server/src/__tests__/attention-service.test.ts ``` Manual steps: 1. Open an issue that has no `reviewPolicy`. Confirm that the Execution properties panel shows no Approvals row. 2. Set the column to `not_creator`. Reload the issue. Confirm that the Approvals row reads "Anyone else", and that the row has no control. 3. Move that issue into review. Open `/decisions`. Confirm that the stalled review card shows the same badge above the review verbs. 4. Let an agent approve the review. Confirm that the activity timeline reads "approved the request" and not "issue thread interaction accepted". Screenshots were captured at 1440x900 and 390x844, in light mode and dark mode, with the three policy states side by side. The leftmost column in each capture is the default. It carries no badge and no extra row. ## Risks Low risk. - The change is additive on screen. Every new surface is behind a constrained policy, so the default path renders exactly as before. - The badge is read-only. It has no control and sends no request, and a test asserts that the row contains no `button`. - An unknown policy value from the wire renders nothing. It does not render the raw enum. - No migration, no schema change, and no endpoint change. ## Model Used Claude Opus 5 (Anthropic), model id `claude-opus-5[1m]`, 1M context window, with extended thinking and tool use enabled. Used through Claude Code for the implementation, the tests, and this description. ## 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 - [ ] 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: Claude Opus 5 (1M context) <noreply@anthropic.com>
191 lines
7.3 KiB
TypeScript
191 lines
7.3 KiB
TypeScript
import type { IssueChangeReceiptEntry } from "@paperclipai/shared";
|
|
import { formatReviewPolicyValue } from "./review-policy";
|
|
|
|
/**
|
|
* Read + format the field-level change receipts carried on an `issue.updated`
|
|
* activity event (the open cross-task write design (audit), built on the field-change receipts the API already records).
|
|
*
|
|
* Every issue PATCH — agent and board alike — must leave an auditable record of
|
|
* who changed what, when, and under which authorization. The server writes that
|
|
* receipt; this module turns it into something a human can scan in the activity
|
|
* stream without opening the audit log.
|
|
*
|
|
* The server already drops `updatedAt` and truncates long text (flagging it with
|
|
* `updated: true`), so this module renders what it is given rather than
|
|
* re-deciding what is interesting.
|
|
*/
|
|
|
|
/** Field names whose raw ids carry no meaning in a scannable summary. */
|
|
const FIELD_LABELS: Record<string, string> = {
|
|
assigneeAgentId: "Assignee",
|
|
assigneeUserId: "Assignee (user)",
|
|
responsibleUserId: "Responsible user",
|
|
blockedByIssueIds: "Blockers",
|
|
labelIds: "Labels",
|
|
parentId: "Parent",
|
|
projectId: "Project",
|
|
goalId: "Goal",
|
|
workMode: "Work mode",
|
|
reviewPolicy: "Who can approve",
|
|
billingCode: "Billing code",
|
|
checkoutRunId: "Checkout run",
|
|
executionRunId: "Execution run",
|
|
hiddenAt: "Hidden",
|
|
startedAt: "Started",
|
|
completedAt: "Completed",
|
|
cancelledAt: "Cancelled",
|
|
requestDepth: "Request depth",
|
|
sourceTrust: "Source trust",
|
|
executionPolicy: "Execution policy",
|
|
executionWorkspaceId: "Execution workspace",
|
|
projectWorkspaceId: "Project workspace",
|
|
};
|
|
|
|
/** Human label for a changed field, e.g. `assigneeAgentId` → "Assignee". */
|
|
export function issueChangeFieldLabel(field: string): string {
|
|
const known = FIELD_LABELS[field];
|
|
if (known) return known;
|
|
// camelCase / snake_case → "Sentence case".
|
|
const spaced = field
|
|
.replace(/([a-z0-9])([A-Z])/g, "$1 $2")
|
|
.replace(/_/g, " ")
|
|
.toLowerCase()
|
|
.trim();
|
|
return spaced.charAt(0).toUpperCase() + spaced.slice(1);
|
|
}
|
|
|
|
const VALUE_PREVIEW_BUDGET = 72;
|
|
|
|
/**
|
|
* Render one side of a change for display. Never returns an empty string, so a
|
|
* receipt row always reads as "from → to" rather than trailing into nothing.
|
|
*/
|
|
export function formatIssueChangeValue(
|
|
value: unknown,
|
|
options: { resolveAgentLabel?: (id: string) => string | null | undefined;
|
|
resolveUserLabel?: (id: string) => string | null | undefined;
|
|
field?: string } = {},
|
|
): string {
|
|
// `reviewPolicy` is nullable-by-default: a cleared column means "anyone can
|
|
// approve", not "no value" (PAP-16506), so it resolves before the null branch.
|
|
if (options.field === "reviewPolicy") return formatReviewPolicyValue(value);
|
|
if (value === null || value === undefined || value === "") return "none";
|
|
if (typeof value === "boolean") return value ? "yes" : "no";
|
|
if (typeof value === "number") return String(value);
|
|
|
|
if (Array.isArray(value)) {
|
|
if (value.length === 0) return "none";
|
|
const strings = value.filter((entry): entry is string => typeof entry === "string");
|
|
if (strings.length !== value.length) return `${value.length} items`;
|
|
return strings.length <= 3
|
|
? strings.map((id) => shortenId(id)).join(", ")
|
|
: `${strings.length} items`;
|
|
}
|
|
|
|
if (value instanceof Date) return value.toLocaleString();
|
|
|
|
if (typeof value === "string") {
|
|
const trimmed = value.trim();
|
|
if (!trimmed) return "none";
|
|
// Ids resolve to names when the directory is loaded; otherwise they shorten.
|
|
const resolved = options.field?.toLowerCase().includes("agent")
|
|
? options.resolveAgentLabel?.(trimmed)
|
|
: options.field?.toLowerCase().includes("user")
|
|
? options.resolveUserLabel?.(trimmed)
|
|
: null;
|
|
if (resolved) return resolved;
|
|
if (isIsoTimestamp(trimmed)) return new Date(trimmed).toLocaleString();
|
|
if (looksLikeId(trimmed)) return shortenId(trimmed);
|
|
const humanized = trimmed.includes(" ") ? trimmed : trimmed.replace(/_/g, " ");
|
|
return truncate(humanized);
|
|
}
|
|
|
|
// Objects (execution policy, workspace settings) are structural — the receipt
|
|
// records that they moved, and the audit log holds the full value.
|
|
return "updated";
|
|
}
|
|
|
|
function truncate(value: string): string {
|
|
const chars = Array.from(value);
|
|
if (chars.length <= VALUE_PREVIEW_BUDGET) return value;
|
|
return `${chars.slice(0, VALUE_PREVIEW_BUDGET).join("")}…`;
|
|
}
|
|
|
|
function isIsoTimestamp(value: string): boolean {
|
|
return /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/.test(value) && !Number.isNaN(Date.parse(value));
|
|
}
|
|
|
|
function looksLikeId(value: string): boolean {
|
|
return /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(value);
|
|
}
|
|
|
|
function shortenId(value: string): string {
|
|
return looksLikeId(value) ? value.slice(0, 8) : truncate(value);
|
|
}
|
|
|
|
export interface IssueChangeReceiptRow {
|
|
field: string;
|
|
label: string;
|
|
from: string;
|
|
to: string;
|
|
/** Server flagged the values as truncated previews of long text. */
|
|
truncated: boolean;
|
|
}
|
|
|
|
function isChangeEntry(value: unknown): value is IssueChangeReceiptEntry {
|
|
return Boolean(value) && typeof value === "object" && !Array.isArray(value) && "from" in (value as object) && "to" in (value as object);
|
|
}
|
|
|
|
/**
|
|
* Parse `details.changes` off an activity event into display rows. Returns an
|
|
* empty array for events with no receipt (older rows, non-PATCH actions), so
|
|
* callers can render nothing without special-casing.
|
|
*/
|
|
export function readIssueChangeReceipt(
|
|
details: Record<string, unknown> | null | undefined,
|
|
options: Parameters<typeof formatIssueChangeValue>[1] = {},
|
|
): IssueChangeReceiptRow[] {
|
|
const changes = details?.changes;
|
|
if (!changes || typeof changes !== "object" || Array.isArray(changes)) return [];
|
|
|
|
const rows: IssueChangeReceiptRow[] = [];
|
|
for (const [field, entry] of Object.entries(changes as Record<string, unknown>)) {
|
|
if (!isChangeEntry(entry)) continue;
|
|
rows.push({
|
|
field,
|
|
label: issueChangeFieldLabel(field),
|
|
from: formatIssueChangeValue(entry.from, { ...options, field }),
|
|
to: formatIssueChangeValue(entry.to, { ...options, field }),
|
|
truncated: entry.updated === true,
|
|
});
|
|
}
|
|
// Stable, scannable order regardless of JSON key order (jsonb reorders keys).
|
|
return rows.sort((a, b) => a.label.localeCompare(b.label));
|
|
}
|
|
|
|
/** Authorization reasons, as recorded by the server's write-policy decision. */
|
|
const AUTHORIZATION_REASON_LABELS: Record<string, string> = {
|
|
allow_visible_issue_write: "default-open write on a visible task",
|
|
allow_scoped_agent_write: "scoped agent write",
|
|
allow_board_actor: "board actor",
|
|
allow_self: "own task",
|
|
allow_issue_mention_grant: "mention grant",
|
|
allow_direct_parent_report: "direct parent report",
|
|
allow_low_trust_boundary: "low-trust boundary allowance",
|
|
allow_explicit_grant: "explicit permission grant",
|
|
allow_instance_admin: "instance admin",
|
|
allow_local_board: "local board",
|
|
internal_agent_write: "internal agent write",
|
|
};
|
|
|
|
/**
|
|
* Human phrasing for the authorization reason on a write receipt. Unknown
|
|
* reasons degrade to their humanized code rather than disappearing — an
|
|
* unexplained write is worse than an ugly one.
|
|
*/
|
|
export function issueAuthorizationReasonLabel(reason: string | null | undefined): string | null {
|
|
const trimmed = reason?.trim();
|
|
if (!trimmed) return null;
|
|
return AUTHORIZATION_REASON_LABELS[trimmed] ?? trimmed.replace(/_/g, " ");
|
|
}
|