From 69027cbaaee7ec4e37dfda7fc9700dea9a28e16a Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 14 Aug 2026 16:44:31 -0700 Subject: [PATCH 001/159] fix(workspaces): reopen archived git worktree for managed_checkout projects (#11395) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Execution workspaces give agent tasks isolated Git worktrees > - Archived isolated workspaces must reopen against a live project checkout > - A managed_checkout project has no project workspace directory in its row > - The reopen path used the removed archived worktree as the Git working directory > - This pull request resolves the live managed checkout and reports a clear error when it is unavailable > - The benefit is reliable workspace reopen behavior after archive cleanup ## Linked Issues or Issue Description Related public pull request: [#6164](https://github.com/paperclipai/paperclip/pull/6164) clears archive state during un-archive. This pull request fixes the separate reopen failure that occurs after archive cleanup. **What happened?** An archived isolated `git_worktree` workspace under a `managed_checkout` project failed to reopen after cleanup. The route attempted to run Git in the removed archived worktree and returned a generic service error. **Expected behavior** The reopen path should use the live managed checkout as the Git base directory and should return a clear error when that directory is unavailable. **Steps to reproduce** 1. Create a project with `managed_checkout` source control. 2. Create and archive an isolated `git_worktree` execution workspace. 3. Let archive cleanup remove the worktree. 4. Reopen the workspace for an issue. **Paperclip version or commit** `cab0c31dc61310106caef42ca244e9f7b0f19460` **Deployment mode** Local dev with the default embedded database. **Agent adapter(s) involved** Not adapter-specific. This issue affects core workspace handling. ## What Changed - Resolve the live managed checkout when a managed project reopens an archived Git worktree. - Keep local-folder projects on their project workspace directory. - Validate the Git base directory before `git rev-parse` and return a scrubbed error. - Add nine regression tests for workspace reopen behavior. ## Verification - `server` TypeScript check passes with `tsc --noEmit`. - `server/src/__tests__/execution-workspace-reopen.test.ts` passes with 9 tests. - GitHub Actions must pass all required PR checks. ## Risks Low risk. The change affects only archived isolated workspace reopen behavior. It reuses the existing managed checkout and Git authentication helpers. It adds no new credential path, endpoint, or telemetry. ## Model Used OpenAI GPT-5 assisted with review and GitHub operations. The implementation author supplied the code and test results. ## 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 --- .../execution-workspace-reopen.test.ts | 145 +++++++++++++++++- .../src/__tests__/workspace-runtime.test.ts | 94 ++++++++++++ server/src/services/execution-workspaces.ts | 38 ++++- server/src/services/workspace-runtime.ts | 19 ++- 4 files changed, 288 insertions(+), 8 deletions(-) diff --git a/server/src/__tests__/execution-workspace-reopen.test.ts b/server/src/__tests__/execution-workspace-reopen.test.ts index ea12f68e853..9ff003a237c 100644 --- a/server/src/__tests__/execution-workspace-reopen.test.ts +++ b/server/src/__tests__/execution-workspace-reopen.test.ts @@ -1,4 +1,6 @@ -import { mkdtemp, rm } from "node:fs/promises"; +import { execFileSync } from "node:child_process"; +import { mkdirSync, writeFileSync } from "node:fs"; +import { mkdtemp, rm, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { randomUUID } from "node:crypto"; @@ -27,6 +29,7 @@ import { readExecutionWorkspaceLifecycleGeneration, readMetadataReopenPendingConsumptionSince, } from "../services/execution-workspaces.js"; +import { resolveManagedProjectWorkspaceDir } from "../home-paths.js"; const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; @@ -128,6 +131,96 @@ describeEmbeddedPostgres("reopen archived isolated execution workspace", () => { return workspaceId; } + // Seed a company and project whose primary project workspace has a null cwd. + // This models a managed_checkout project: the base is not a local folder, so + // the live managed checkout supplies the base path at rebuild time. + async function seedManagedCheckoutProject() { + const companyId = randomUUID(); + const projectId = randomUUID(); + const projectWorkspaceId = randomUUID(); + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `PAP-${companyId.slice(0, 8)}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(projects).values({ + id: projectId, + companyId, + name: "Managed checkout project", + status: "in_progress", + }); + await db.insert(projectWorkspaces).values({ + id: projectWorkspaceId, + companyId, + projectId, + name: "Primary", + sourceType: "managed_checkout", + cwd: null, + isPrimary: true, + }); + return { companyId, projectId, projectWorkspaceId }; + } + + // Seed one closed isolated git_worktree row. The reaper already removed the + // worktree directory, so cwd points at a path that is not on disk. + async function seedClosedGitWorktreeWorkspace(input: { + companyId: string; + projectId: string; + projectWorkspaceId: string; + cwd: string; + repoUrl: string; + branchName: string; + }) { + const workspaceId = randomUUID(); + await db.insert(executionWorkspaces).values({ + id: workspaceId, + companyId: input.companyId, + projectId: input.projectId, + projectWorkspaceId: input.projectWorkspaceId, + mode: "isolated_workspace", + strategyType: "git_worktree", + name: "reopen-git-worktree", + status: "archived", + providerType: "local_fs", + cwd: input.cwd, + repoUrl: input.repoUrl, + baseRef: null, + branchName: input.branchName, + closedAt: new Date(), + cleanupReason: "issue_terminal", + cleanupEligibleAt: new Date(), + metadata: { + [EXECUTION_WORKSPACE_LIFECYCLE_GENERATION_METADATA_KEY]: 1, + }, + }); + return workspaceId; + } + + // Build a real git repository at the managed checkout path so + // ensureManagedProjectWorkspace adopts it without a network clone. The repo + // holds the branch that the archived worktree row references. + function initManagedGitRepo(dir: string, worktreeBranch: string) { + mkdirSync(dir, { recursive: true }); + const git = (...args: string[]) => + execFileSync("git", args, { + cwd: dir, + env: { + ...process.env, + GIT_AUTHOR_NAME: "Test", + GIT_AUTHOR_EMAIL: "test@example.com", + GIT_COMMITTER_NAME: "Test", + GIT_COMMITTER_EMAIL: "test@example.com", + }, + stdio: "ignore", + }); + git("init"); + writeFileSync(join(dir, "README.md"), "seed\n"); + git("add", "README.md"); + git("commit", "-m", "seed"); + git("branch", worktreeBranch); + } + async function seedIssue(input: { companyId: string; projectId: string; @@ -246,6 +339,56 @@ describeEmbeddedPostgres("reopen archived isolated execution workspace", () => { expect(readExecutionWorkspaceLifecycleGeneration(row?.metadata as Record | null)).toBe(3); }); + it("resolves the managed base checkout for a git_worktree row when the project workspace cwd is null", async () => { + const previousHome = process.env.PAPERCLIP_HOME; + const tempHome = await mkdtemp(join(tmpdir(), "paperclip-reopen-home-")); + tempDirs.push(tempHome); + process.env.PAPERCLIP_HOME = tempHome; + try { + const { companyId, projectId, projectWorkspaceId } = await seedManagedCheckoutProject(); + const repoUrl = "https://example.test/acme/widget.git"; + const branchName = "reopen-feature"; + // Build the live managed checkout that the rebuild must spawn git in. + const managedDir = resolveManagedProjectWorkspaceDir({ companyId, projectId, repoName: "widget" }); + initManagedGitRepo(managedDir, branchName); + + // The archived worktree path. The reaper already removed it from disk. + const deletedWorktree = join(tempHome, "worktrees", "reopen-worktree"); + const workspaceId = await seedClosedGitWorktreeWorkspace({ + companyId, + projectId, + projectWorkspaceId, + cwd: deletedWorktree, + repoUrl, + branchName, + }); + const issueId = await seedIssue({ companyId, projectId, workspaceId, issueNumber: 4305 }); + + const svc = executionWorkspaceService(db); + const result = await svc.reopenClosedIsolatedExecutionWorkspaceForIssue({ + workspaceId, + issue: { id: issueId, companyId, projectId }, + actor: { agentId: null, actorType: "user" }, + }); + + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.reopened).toBe(true); + + const row = await readWorkspace(workspaceId); + expect(row?.status).toBe("active"); + expect(row?.closedAt).toBeNull(); + expect(row?.cleanupReason).toBeNull(); + // The rebuild recreated the worktree at the archived path from the managed + // base checkout. + const worktreeStat = await stat(deletedWorktree).catch(() => null); + expect(worktreeStat?.isDirectory()).toBe(true); + } finally { + if (previousHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = previousHome; + } + }); + it("refuses to reopen a workspace in another company", async () => { const first = await seedCompanyProject(); const second = await seedCompanyProject(); diff --git a/server/src/__tests__/workspace-runtime.test.ts b/server/src/__tests__/workspace-runtime.test.ts index cf052cb4372..b5e97ee1264 100644 --- a/server/src/__tests__/workspace-runtime.test.ts +++ b/server/src/__tests__/workspace-runtime.test.ts @@ -3127,6 +3127,100 @@ describe("realizeExecutionWorkspace", () => { await expect(fs.readFile(path.join(initial.cwd, ".paperclip-restored-state"), "utf8")).resolves.toBe("reprovisioned\n"); }, 15_000); + it("rejects an empty base checkout path with a clear cause", async () => { + // An empty base path makes the later "git" spawn fail with a raw ENOENT. + // The reopen must throw a clear cause first that names the empty checkout. + let error: unknown = null; + try { + await ensurePersistedExecutionWorkspaceAvailable({ + base: { + baseCwd: "", + source: "project_primary", + projectId: "project-1", + workspaceId: "workspace-1", + repoUrl: null, + repoRef: "HEAD", + }, + workspace: { + mode: "isolated_workspace", + strategyType: "git_worktree", + cwd: "/does-not-exist/worktree", + providerRef: "/does-not-exist/worktree", + projectId: "project-1", + projectWorkspaceId: "workspace-1", + repoUrl: null, + baseRef: "HEAD", + branchName: "feature-branch", + }, + issue: { + id: "issue-1", + identifier: "PAP-461", + title: "Empty base checkout path", + }, + agent: { + id: "agent-1", + name: "Codex Coder", + companyId: "company-1", + }, + }); + } catch (err) { + error = err; + } + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toBe( + "Cannot rebuild the git worktree: the base project checkout path is empty.", + ); + }); + + it("preserves a whitespace-only base checkout path and reports it as missing", async () => { + // The reopen must use the persisted base path exactly. A trim would change + // a whitespace-only path into an empty path and hide the real cause. + // A directory name can consist of spaces, so keep the path unchanged and + // let the directory-exists check report the missing checkout. + let error: unknown = null; + try { + await ensurePersistedExecutionWorkspaceAvailable({ + base: { + baseCwd: " ", + source: "project_primary", + projectId: "project-1", + workspaceId: "workspace-1", + repoUrl: null, + repoRef: "HEAD", + }, + workspace: { + mode: "isolated_workspace", + strategyType: "git_worktree", + cwd: "/does-not-exist/worktree", + providerRef: "/does-not-exist/worktree", + projectId: "project-1", + projectWorkspaceId: "workspace-1", + repoUrl: null, + baseRef: "HEAD", + branchName: "feature-branch", + }, + issue: { + id: "issue-1", + identifier: "PAP-461", + title: "Whitespace base checkout path", + }, + agent: { + id: "agent-1", + name: "Codex Coder", + companyId: "company-1", + }, + }); + } catch (err) { + error = err; + } + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toBe( + "Cannot rebuild the git worktree: the base project checkout directory does not exist.", + ); + }); + it("auto-detects the default branch when baseRef is not configured", async () => { // Create a repo with "master" as default branch (not "main") const repoRoot = await createTempRepo("master"); diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index 4e32a69a37f..2a3cddc4d05 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -52,6 +52,7 @@ import { type PullRequestMergeDetailsResolver, } from "./github-pull-request-merge.js"; import { visibleIssueCondition } from "./issue-visibility.js"; +import { createGitRemoteAuthProvider } from "./git-credentials.js"; import { readProjectWorkspaceRuntimeConfig } from "./project-workspace-runtime-config.js"; import { listCurrentRuntimeServicesForExecutionWorkspaces, @@ -2764,11 +2765,17 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic }; } - const [{ ensurePersistedExecutionWorkspaceAvailable }, { workspaceOperationService }] = - await Promise.all([ - import("./workspace-runtime.js"), - import("./workspace-operations.js"), - ]); + const [ + { ensurePersistedExecutionWorkspaceAvailable }, + { workspaceOperationService }, + { ensureManagedProjectWorkspace }, + ] = await Promise.all([ + import("./workspace-runtime.js"), + import("./workspace-operations.js"), + // heartbeat.js imports this module, so a static import creates a + // cycle. Load ensureManagedProjectWorkspace dynamically instead. + import("./heartbeat.js"), + ]); const [projectWorkspace, projectPolicy] = await Promise.all([ row.projectWorkspaceId ? db @@ -2786,6 +2793,25 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic .where(and(eq(projects.companyId, row.companyId), eq(projects.id, row.projectId))) .then((rows) => parseProjectExecutionWorkspacePolicy(rows[0]?.executionWorkspacePolicy)), ]); + // Resolve the base checkout that the rebuild spawns git in. A + // local-folder project stores its base path in projectWorkspaces.cwd. + // A managed_checkout project stores null there, so resolve its live + // managed checkout instead. Never use row.cwd for a git_worktree + // rebuild: row.cwd is the archived worktree path, which the reaper + // already removed from disk. A spawn in that missing directory fails + // with "spawn git ENOENT" and hides the real cause. + let resolvedBaseCwd = projectWorkspace?.cwd ?? null; + if (resolvedBaseCwd == null && row.strategyType === "git_worktree" && row.projectId) { + const managedWorkspace = await ensureManagedProjectWorkspace({ + companyId: row.companyId, + projectId: row.projectId, + repoUrl: row.repoUrl, + resolveGitAuth: createGitRemoteAuthProvider(db, row.companyId, { + issueId: row.sourceIssueId ?? issue.id, + }), + }); + resolvedBaseCwd = managedWorkspace.cwd; + } const config = readExecutionWorkspaceConfig(row.metadata as Record | null); const nextGeneration = readExecutionWorkspaceLifecycleGeneration( row.metadata as Record | null, @@ -2803,7 +2829,7 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic const realized = await ensurePersistedExecutionWorkspaceAvailable({ db: tx as unknown as Db, base: { - baseCwd: projectWorkspace?.cwd ?? row.cwd ?? "", + baseCwd: resolvedBaseCwd ?? row.cwd ?? "", source: "task_session", projectId: row.projectId, workspaceId: row.projectWorkspaceId, diff --git a/server/src/services/workspace-runtime.ts b/server/src/services/workspace-runtime.ts index cb96c5f9d4d..563b2f6db64 100644 --- a/server/src/services/workspace-runtime.ts +++ b/server/src/services/workspace-runtime.ts @@ -3031,7 +3031,24 @@ export async function ensurePersistedExecutionWorkspaceAvailable(input: { } return realized; } - const repoRoot = await runGit(["rev-parse", "--show-toplevel"], input.base.baseCwd); + // Validate the base checkout before the git spawn. A missing or empty base + // path makes the "git" spawn fail with a raw "spawn git ENOENT" error. That + // error hides the real cause: the base project checkout is not on disk. + // Throw a clear cause first so a future failure names the missing checkout. + // Keep the persisted path exact. A directory name can start or end with a + // space, so a trim would change a valid checkout path. + const baseCwd = asString(input.base.baseCwd, ""); + if (!baseCwd) { + throw new Error( + "Cannot rebuild the git worktree: the base project checkout path is empty.", + ); + } + if (!await directoryExists(baseCwd)) { + throw new Error( + "Cannot rebuild the git worktree: the base project checkout directory does not exist.", + ); + } + const repoRoot = await runGit(["rev-parse", "--show-toplevel"], baseCwd); const recordedBaseRefSha = readRecordedBaseRefSha(input.workspace.metadata); if (await directoryExists(cwd)) { const reuseBaseRef = input.workspace.baseRef ?? input.base.repoRef ?? null; From 8ee1fb21a6538cd214b6c4a95c9ae7678f8bc297 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:29:36 -0400 Subject: [PATCH 002/159] feat(ui): badge the review policy when it constrains approval (#10938) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) --- .../src/__tests__/attention-service.test.ts | 10 +- server/src/services/attention.ts | 8 ++ ui/src/components/AttentionQueueRow.test.tsx | 84 +++++++++++++++++ ui/src/components/AttentionQueueRow.tsx | 2 + ui/src/components/IssueProperties.test.tsx | 68 ++++++++++++++ ui/src/components/StalledReviewActions.tsx | 27 +++++- .../issue-properties/IssueProperties.tsx | 15 +++ .../issue-properties/primitives.tsx | 5 +- ui/src/lib/activity-format.test.ts | 66 +++++++++++++ ui/src/lib/activity-format.ts | 93 +++++++++++++++++++ ui/src/lib/issue-change-receipt.test.ts | 9 ++ ui/src/lib/issue-change-receipt.ts | 5 + ui/src/lib/review-policy.test.ts | 64 +++++++++++++ ui/src/lib/review-policy.ts | 88 ++++++++++++++++++ 14 files changed, 541 insertions(+), 3 deletions(-) create mode 100644 ui/src/lib/review-policy.test.ts create mode 100644 ui/src/lib/review-policy.ts diff --git a/server/src/__tests__/attention-service.test.ts b/server/src/__tests__/attention-service.test.ts index b2cd6bc6497..88847833ff8 100644 --- a/server/src/__tests__/attention-service.test.ts +++ b/server/src/__tests__/attention-service.test.ts @@ -172,6 +172,7 @@ describeEmbeddedPostgres("attention service", () => { unblockDescriptor?: { owner: { userId: string } | "board"; action: string } | null; blockedTransitionAt?: Date | null; harnessKind?: string | null; + reviewPolicy?: "anyone" | "not_creator" | "human_only" | null; }) { const id = input.id ?? randomUUID(); await db.insert(issues).values({ @@ -181,6 +182,7 @@ describeEmbeddedPostgres("attention service", () => { title: input.title, status: input.status, priority: input.priority ?? "medium", + reviewPolicy: input.reviewPolicy ?? null, parentId: input.parentId ?? null, projectId: input.projectId ?? null, projectWorkspaceId: input.projectWorkspaceId ?? null, @@ -314,6 +316,9 @@ describeEmbeddedPostgres("attention service", () => { title: "Stalled review blocker", status: "in_review", assigneeAgentId: reviewerId, + // PAP-16506: the /decisions review card states who may give the verdict, + // so the subject has to carry the issue's opt-in constraint. + reviewPolicy: "human_only", updatedAt: new Date("2026-07-09T12:05:00.000Z"), }); await db.insert(issueRelations).values({ @@ -673,7 +678,10 @@ describeEmbeddedPostgres("attention service", () => { // A stalled review resolves in-row on the /decisions card (PAP-16080 §4.4). inlineResolvable: true, subject: expect.objectContaining({ - metadata: expect.objectContaining({ reviewAttentionState: "stalled" }), + metadata: expect.objectContaining({ + reviewAttentionState: "stalled", + reviewPolicy: "human_only", + }), }), decisionVerbs: expect.arrayContaining([ expect.objectContaining({ id: "choose_review_path", label: "Choose review path" }), diff --git a/server/src/services/attention.ts b/server/src/services/attention.ts index 0b4b72688e3..123b73411b9 100644 --- a/server/src/services/attention.ts +++ b/server/src/services/attention.ts @@ -43,6 +43,7 @@ import type { AttentionSubject, AttentionTriageAttribution, AttentionWorkspaceRef, + IssueReviewPolicy, } from "@paperclipai/shared"; import { badRequest } from "../errors.js"; import { PRODUCTIVITY_REVIEW_ORIGIN_KIND } from "./productivity-review.js"; @@ -123,6 +124,8 @@ type IssueSummaryRow = { title: string; status: string; priority: string; + /** Who may give the `in_review` verdict; `null`/absent ≡ "anyone" (PAP-16506). */ + reviewPolicy?: IssueReviewPolicy | null; assigneeAgentId: string | null; assigneeUserId: string | null; createdAt: Date; @@ -341,6 +344,9 @@ function issueSubject(prefix: string, issue: IssueSubjectRow): AttentionSubject priority: issue.priority, assigneeAgentId: issue.assigneeAgentId, assigneeUserId: issue.assigneeUserId, + // Only present when the row was selected with the column, so subjects + // built from narrower selects do not claim a policy they never read. + ...(issue.reviewPolicy !== undefined ? { reviewPolicy: issue.reviewPolicy } : {}), }, }; } @@ -776,6 +782,7 @@ async function issueSummaryMap(db: Db, companyId: string, issueIds: Array { expect(el.textContent).toContain("Send back to work"); }); + // PAP-16506 P4: an opt-in policy warns before the card offers Approve, because + // the server refuses the verdict. The default earns no pixels. + const reviewItemWithPolicy = (reviewPolicy?: unknown) => + buildItem({ + sourceKind: "review" as AttentionSourceKind, + inlineResolvable: true, + subject: { + kind: "issue", + id: "issue-1", + companyId: "c1", + title: "PR ready for review", + identifier: null, + status: "in_review", + href: "/PAP/issues/PAP-1", + metadata: { + reviewAttentionState: "stalled", + ...(reviewPolicy === undefined ? {} : { reviewPolicy }), + }, + }, + }); + + it("says nothing about who can approve on a review with no policy set", () => { + const el = render( + , + ); + expect(el.querySelector('[data-testid="review-policy-badge"]')).toBeNull(); + expect(el.textContent).not.toContain("Anyone else"); + expect(el.textContent).not.toContain("Human only"); + // The verbs still render — suppressing the badge must not suppress the card. + expect(el.textContent).toContain("Send back to work"); + }); + + it("badges the opt-in constraint when the issue carries one", () => { + const el = render( + , + ); + const badge = el.querySelector('[data-testid="review-policy-badge"]'); + expect(badge?.getAttribute("data-review-policy")).toBe("human_only"); + expect(badge?.textContent).toContain("Human only"); + expect(badge?.getAttribute("title")).toContain("Agents cannot"); + }); + + it("badges a not_creator policy as 'Anyone else'", () => { + const el = render( + , + ); + const badge = el.querySelector('[data-testid="review-policy-badge"]'); + expect(badge?.getAttribute("data-review-policy")).toBe("not_creator"); + expect(badge?.textContent).toContain("Anyone else"); + }); + + it("shows no badge for an explicit default or an unrecognised policy", () => { + for (const policy of ["anyone", "board_only"]) { + const el = render( + , + ); + expect(el.querySelector('[data-testid="review-policy-badge"]')).toBeNull(); + } + }); + it("deep-links a covered review instead of inlining", () => { const el = render( ); } diff --git a/ui/src/components/IssueProperties.test.tsx b/ui/src/components/IssueProperties.test.tsx index 0427bfb6bce..ee3a1a69004 100644 --- a/ui/src/components/IssueProperties.test.tsx +++ b/ui/src/components/IssueProperties.test.tsx @@ -2900,6 +2900,74 @@ describe("IssueProperties", () => { Array.from(container.querySelectorAll("button")).some((button) => button.textContent?.includes("Unarchive")), ).toBe(false); + act(() => root.unmount()); + }); + // PAP-16506 P4: only an agent sets `reviewPolicy`, so the panel shows it and + // never offers a control. The default — a NULL column, meaning anyone with + // write access can approve — is what every issue already does, so it shows + // nothing at all rather than a row reading "Anyone" or "None". + const findApprovalsRow = () => + container.querySelector('[data-property-label="Approvals"]')?.closest('[data-property-row="true"]') ?? null; + + it("shows no approvals row on an issue that has never set a policy", async () => { + const root = renderProperties(container, { + issue: createIssue({ reviewPolicy: null }), + childIssues: [], + onUpdate: vi.fn(), + inline: true, + }); + await flush(); + + expect(findApprovalsRow()).toBeNull(); + expect(container.textContent).not.toContain("Review policy"); + + act(() => root.unmount()); + }); + + it("shows no approvals row when the policy is the explicit default", async () => { + const root = renderProperties(container, { + issue: createIssue({ reviewPolicy: "anyone" }), + childIssues: [], + onUpdate: vi.fn(), + inline: true, + }); + await flush(); + + expect(findApprovalsRow()).toBeNull(); + + act(() => root.unmount()); + }); + + it("badges an opt-in constraint read-only, with no control to change it", async () => { + const onUpdate = vi.fn(); + const root = renderProperties(container, { + issue: createIssue({ reviewPolicy: "human_only" }), + childIssues: [], + onUpdate, + inline: true, + }); + await flush(); + + const row = findApprovalsRow(); + expect(row?.textContent).toContain("Human only"); + // Read-only: the row is a chip, not a picker — nothing here can PATCH. + expect(row?.querySelector("button")).toBeNull(); + expect(onUpdate).not.toHaveBeenCalled(); + + act(() => root.unmount()); + }); + + it("badges a not_creator policy as 'Anyone else'", async () => { + const root = renderProperties(container, { + issue: createIssue({ reviewPolicy: "not_creator" }), + childIssues: [], + onUpdate: vi.fn(), + inline: true, + }); + await flush(); + + expect(findApprovalsRow()?.textContent).toContain("Anyone else"); + act(() => root.unmount()); }); }); diff --git a/ui/src/components/StalledReviewActions.tsx b/ui/src/components/StalledReviewActions.tsx index ab3aadc5624..d2b801c6277 100644 --- a/ui/src/components/StalledReviewActions.tsx +++ b/ui/src/components/StalledReviewActions.tsx @@ -1,11 +1,13 @@ import { useState, type ReactNode } from "react"; import { useMutation, useQueryClient } from "@tanstack/react-query"; import { CheckCircle2, Loader2, RotateCcw, Undo2 } from "lucide-react"; -import type { StalledReviewDecisionAction } from "@paperclipai/shared"; +import type { IssueReviewPolicy, StalledReviewDecisionAction } from "@paperclipai/shared"; import { issuesApi } from "../api/issues"; import { useToastActions } from "../context/ToastContext"; import { queryKeys } from "../lib/queryKeys"; +import { issueReviewPolicyBadge } from "../lib/review-policy"; import { cn } from "../lib/utils"; +import { Badge } from "./ui/badge"; import { Button } from "./ui/button"; import { Textarea } from "./ui/textarea"; @@ -16,6 +18,12 @@ interface StalledReviewActionsProps { footerSlot?: ReactNode; /** Fired after a decision lands so the surface can navigate / close / refetch extras. */ onResolved?: (action: StalledReviewDecisionAction) => void; + /** + * The issue's `reviewPolicy` (PAP-16506). Only an opt-in constraint is shown; + * the default — `null`/`"anyone"` — renders nothing, because "anyone can + * approve" is what every issue already does. + */ + reviewPolicy?: IssueReviewPolicy | null; className?: string; } @@ -40,6 +48,7 @@ export function StalledReviewActions({ companyId, footerSlot, onResolved, + reviewPolicy, className, }: StalledReviewActionsProps) { const queryClient = useQueryClient(); @@ -73,9 +82,25 @@ export function StalledReviewActions({ const noteEmpty = note.trim().length === 0; const runningFor = (action: StalledReviewDecisionAction) => pending && decide.variables === action; + const policyBadge = issueReviewPolicyBadge(reviewPolicy); return (
+ {/* Only a constrained policy gets a badge: the server refuses a verdict + from the wrong actor, so the card has to warn before offering Approve. + The default needs no line — anyone with write access can approve. */} + {policyBadge ? ( + + + {policyBadge.label} + + ) : null}