mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-30 21:49:18 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents move issues to `in_review` and rely on a "review path" (an interaction, an approval, a monitor, or a named reviewer) to tell them who decides next. > - That review path can silently disappear. A user comment supersedes the pending interaction, a monitor is exhausted, or a run ends without restoring a path. The issue then sits in `in_review` with nobody reviewing it and no visible action. > - Such issues become invisible zombies. Nobody knows a decision is owed, so the work stalls forever. > - This pull request makes the review path a maintained invariant, exposes a `reviewAttention` surface, and gives every stalled review three inline actions in the UI. > - The benefit is that an `in_review` issue always shows who reviews it, or shows an amber "nobody is reviewing this" notice with one-click Approve, Request changes, and Send back to work. ## Linked Issues or Issue Description This pull request describes the problem inline. The tracking issue is internal. **Subsystem affected** The review and attention loop that agents and humans share: the `in_review` status, the `reviewAttention` surface, the /decisions attention feed, and the issue-page review panel. **Problem or motivation** Agent-owned issues in `in_review` can lose their last review path. A user comment supersedes the pending interaction. A monitor is exhausted. A run ends without restoring a path. The issue then sits in `in_review` with no reviewer and no visible action. It becomes an invisible zombie and the work never progresses. **Proposed solution** Maintain the review path as a server invariant. Expose a `reviewAttention` field that says what is under review, who decides, and since when. Render a persistent review panel on the issue page and inline actions on the /decisions feed. Keep human PATCHes into `in_review` ungated, but record the requesting user so the panel never renders empty. **Alternatives considered** A pure background auto-recovery sweep. This stays opt-in and is not enough on its own, because it is invisible to the human. A bare status banner. This is rejected, because it gives no action to resolve the stall. **Roadmap alignment** This improves the core review and attention loop that both agents and humans use every day. ## What Changed - **Server — maintained review-path invariant:** when an issue enters or sits in `in_review`, the server derives and persists a review path (interaction, approval, monitor, or the requesting user) and recovers a stale path with one bounded wake instead of leaving the issue pathless. - **Server — `reviewAttention` surface:** a new field describes what is under review (bound target with links), who decides, since when, and whether the review is stalled. Stalled agent-assigned reviews are now included in the attention feed. - **Server — inline stalled-review decisions:** secured routes let a permitted responder Approve (→ `done`), Request changes (→ `todo` + wake carrying the note), or Send back to work (→ `todo` + wake) directly from the attention feed. - **Server — resume-intent wake:** an `in_review -> todo` transition now wakes the assigned agent so a resumed review is not dropped. - **Server — user-entry symmetry:** user PATCHes into `in_review` stay ungated (no 422 for humans) and record the requesting user, who becomes the named responder when no other path exists. - **UI — review panel:** a persistent `IssueReviewPanel` renders above the thread whenever status is `in_review`. The covered state shows the bound target, responder, and outcomes and hoists the pending interaction/approval card. The stalled state shows the amber notice plus the three actions. - **UI — decisions card actions:** the same three actions render inline on the /decisions `AttentionQueueRow`. - **UI — responsive fix:** the stalled action row stacks to full-width buttons at phone width and returns to a horizontal row at `sm` and up. New 390px stories capture the phone layout. ## Verification - `cd ui && npx vitest run src/components/IssueReviewPanel.test.tsx src/components/AttentionQueueRow.test.tsx src/lib/attention.test.ts src/api/issues.test.ts` — 91 tests pass. - Server suites added and updated: `issue-review-attention`, `issue-stalled-review-decision-routes`, `review-path-recovery`, `recovery-observability`, and related route/liveness tests (run by CI). - A designer reviewed the UI at 390px and desktop in light and dark themes on both the issue-page panel and the /decisions card. The stalled action row stacks cleanly at phone width with no overlap and keeps the horizontal row on desktop. ## Risks - **Migration:** adds migration `0200` (next after master `0199`, no renumber). It extends the agent-wakeup-requests schema and is additive. - **Behavioral shift:** `in_review -> todo` now dispatches a wake. This is intended (resume intent) and covered by tests. - **Authz:** the inline decision routes are permission-gated. Only a permitted responder sees and can trigger the actions. - Overall risk is moderate and contained to the review and attention loop. ## Model Used - Claude, Opus 4.8 (`claude-opus-4-8`), extended thinking, tool use and code execution. ## 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 - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
350 lines
11 KiB
TypeScript
350 lines
11 KiB
TypeScript
import { randomUUID } from "node:crypto";
|
|
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
|
import { eq, sql } from "drizzle-orm";
|
|
import {
|
|
activityLog,
|
|
agents,
|
|
agentWakeupRequests,
|
|
agentRuntimeState,
|
|
companySkills,
|
|
companies,
|
|
createDb,
|
|
documentRevisions,
|
|
documents,
|
|
heartbeatRunEvents,
|
|
heartbeatRuns,
|
|
instanceSettings,
|
|
issueComments,
|
|
issueDocuments,
|
|
issues,
|
|
} from "@paperclipai/db";
|
|
import {
|
|
getEmbeddedPostgresTestSupport,
|
|
startEmbeddedPostgresTestDatabase,
|
|
} from "./helpers/embedded-postgres.js";
|
|
import { drainHeartbeatRunsToQuiescence } from "./helpers/drain-heartbeat-runs.js";
|
|
import { heartbeatService, resolveHeartbeatSchedulingSuppression } from "../services/heartbeat.ts";
|
|
import { instanceSettingsService } from "../services/instance-settings.ts";
|
|
|
|
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
|
|
const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
|
|
|
|
if (!embeddedPostgresSupport.supported) {
|
|
console.warn(
|
|
`Skipping embedded Postgres heartbeat worktree suppression tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`,
|
|
);
|
|
}
|
|
|
|
describeEmbeddedPostgres("heartbeat worktree suppression", () => {
|
|
let db!: ReturnType<typeof createDb>;
|
|
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
|
|
|
function isHeartbeatRunDependentFkError(error: unknown) {
|
|
const message = error instanceof Error ? `${error.message} ${String(error.cause ?? "")}` : String(error);
|
|
return (
|
|
message.includes("heartbeat_run_events_run_id_heartbeat_runs_id_fk") ||
|
|
message.includes("activity_log_run_id_heartbeat_runs_id_fk")
|
|
);
|
|
}
|
|
|
|
async function deleteHeartbeatRunsWithDependents() {
|
|
for (let attempt = 0; attempt < 5; attempt += 1) {
|
|
await db.delete(heartbeatRunEvents);
|
|
await db.delete(activityLog);
|
|
try {
|
|
await db.delete(heartbeatRuns);
|
|
return;
|
|
} catch (error) {
|
|
if (!isHeartbeatRunDependentFkError(error) || attempt === 4) throw error;
|
|
await new Promise((resolve) => setTimeout(resolve, 25));
|
|
}
|
|
}
|
|
}
|
|
|
|
beforeAll(async () => {
|
|
tempDb = await startEmbeddedPostgresTestDatabase("heartbeat-worktree-suppression-");
|
|
db = createDb(tempDb.connectionString);
|
|
}, 20_000);
|
|
|
|
afterEach(async () => {
|
|
// Await every in-flight background heartbeat run to quiescence before the
|
|
// deletes below. A live wakeup claims a run and dispatches its execution
|
|
// fire-and-forget, so a run or wakeup can still write heartbeat_runs and
|
|
// issues rows when teardown starts and would race the deletes. The heartbeat
|
|
// service tracks in-flight run and wakeup promises in module state shared
|
|
// across service instances, so a fresh instance here drains the runs the
|
|
// per-test instances dispatched.
|
|
await drainHeartbeatRunsToQuiescence(db, heartbeatService(db));
|
|
await db.delete(issueComments);
|
|
await db.delete(issueDocuments);
|
|
await db.delete(documentRevisions);
|
|
await db.delete(documents);
|
|
await db.delete(activityLog);
|
|
await deleteHeartbeatRunsWithDependents();
|
|
await db.delete(agentWakeupRequests);
|
|
await db.delete(issues);
|
|
await db.delete(agentRuntimeState);
|
|
await db.delete(companySkills);
|
|
await db.delete(agents);
|
|
await db.delete(companies);
|
|
await db.delete(instanceSettings);
|
|
});
|
|
|
|
afterAll(async () => {
|
|
await tempDb?.cleanup();
|
|
}, 60_000);
|
|
|
|
async function insertAgentAndIssue() {
|
|
const companyId = randomUUID();
|
|
const agentId = randomUUID();
|
|
const issueId = randomUUID();
|
|
|
|
await db.insert(companies).values({
|
|
id: companyId,
|
|
name: "Paperclip",
|
|
status: "active",
|
|
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
|
|
requireBoardApprovalForNewAgents: false,
|
|
defaultResponsibleUserId: "responsible-user",
|
|
});
|
|
|
|
await db.insert(agents).values({
|
|
id: agentId,
|
|
companyId,
|
|
name: "Worktree Agent",
|
|
role: "engineer",
|
|
status: "idle",
|
|
adapterType: "process",
|
|
adapterConfig: {
|
|
command: process.execPath,
|
|
args: ["-e", "process.exit(0)"],
|
|
},
|
|
runtimeConfig: {
|
|
heartbeat: {
|
|
enabled: true,
|
|
intervalSec: 60,
|
|
wakeOnDemand: true,
|
|
},
|
|
},
|
|
permissions: {},
|
|
});
|
|
|
|
await db.insert(issues).values({
|
|
id: issueId,
|
|
companyId,
|
|
title: "Assigned work",
|
|
status: "todo",
|
|
priority: "high",
|
|
assigneeAgentId: agentId,
|
|
responsibleUserId: "responsible-user",
|
|
});
|
|
|
|
return { companyId, agentId, issueId };
|
|
}
|
|
|
|
async function armWorktreeRunExecution(cutoff: Date) {
|
|
await instanceSettingsService(db, {
|
|
runtimeEnv: {
|
|
PAPERCLIP_IN_WORKTREE: "true",
|
|
PAPERCLIP_INSTANCE_ID: "test-worktree",
|
|
},
|
|
now: () => cutoff,
|
|
}).updateExperimental({ enableWorktreeRunExecution: true });
|
|
}
|
|
|
|
async function waitForCompletedRun(runId: string, agentId: string) {
|
|
let latestStatus: string | null = null;
|
|
let latestLastRunId: string | null = null;
|
|
|
|
for (let attempt = 0; attempt < 100; attempt += 1) {
|
|
const run = await db
|
|
.select({ status: heartbeatRuns.status })
|
|
.from(heartbeatRuns)
|
|
.where(eq(heartbeatRuns.id, runId))
|
|
.then((rows) => rows[0] ?? null);
|
|
const state = await db
|
|
.select({ lastRunId: agentRuntimeState.lastRunId })
|
|
.from(agentRuntimeState)
|
|
.where(eq(agentRuntimeState.agentId, agentId))
|
|
.then((rows) => rows[0] ?? null);
|
|
|
|
latestStatus = run?.status ?? null;
|
|
latestLastRunId = state?.lastRunId ?? null;
|
|
|
|
if (run && run.status !== "queued" && run.status !== "running" && state?.lastRunId === runId) {
|
|
return run.status;
|
|
}
|
|
|
|
await new Promise((resolve) => setTimeout(resolve, 50));
|
|
}
|
|
|
|
throw new Error(
|
|
`Timed out waiting for heartbeat run ${runId} to finish; latest status=${latestStatus ?? "missing"}, runtime lastRunId=${latestLastRunId ?? "missing"}`,
|
|
);
|
|
}
|
|
|
|
it("suppresses new assignment wakes in worktree instances without creating heartbeat runs", async () => {
|
|
const { agentId, issueId } = await insertAgentAndIssue();
|
|
const heartbeat = heartbeatService(db, {
|
|
runtimeEnv: { PAPERCLIP_IN_WORKTREE: "true" },
|
|
});
|
|
|
|
const run = await heartbeat.wakeup(agentId, {
|
|
source: "assignment",
|
|
triggerDetail: "system",
|
|
reason: "issue_assigned",
|
|
payload: { issueId },
|
|
contextSnapshot: { issueId, wakeReason: "issue_assigned" },
|
|
requestedByActorType: "system",
|
|
requestedByActorId: "issue_assignment",
|
|
});
|
|
|
|
expect(run).toBeNull();
|
|
|
|
const runs = await db.select().from(heartbeatRuns);
|
|
expect(runs).toHaveLength(0);
|
|
|
|
const wakeup = await db
|
|
.select({
|
|
status: agentWakeupRequests.status,
|
|
reason: agentWakeupRequests.reason,
|
|
payload: agentWakeupRequests.payload,
|
|
})
|
|
.from(agentWakeupRequests)
|
|
.then((rows) => rows[0] ?? null);
|
|
expect(wakeup).toMatchObject({
|
|
status: "skipped",
|
|
reason: "heartbeat.scheduling_suppressed",
|
|
});
|
|
expect(wakeup?.payload).toMatchObject({
|
|
issueId,
|
|
heartbeatSkip: { reason: "worktree_instance" },
|
|
});
|
|
});
|
|
|
|
it("does not replay copied queued runs or timer wakes while worktree scheduling is suppressed", async () => {
|
|
const { companyId, agentId, issueId } = await insertAgentAndIssue();
|
|
const runId = randomUUID();
|
|
await db.insert(heartbeatRuns).values({
|
|
id: runId,
|
|
companyId,
|
|
agentId,
|
|
invocationSource: "assignment",
|
|
triggerDetail: "system",
|
|
status: "queued",
|
|
responsibleUserId: "responsible-user",
|
|
contextSnapshot: { issueId, wakeReason: "issue_assigned" },
|
|
});
|
|
|
|
const heartbeat = heartbeatService(db, {
|
|
runtimeEnv: { PAPERCLIP_IN_WORKTREE: "true" },
|
|
});
|
|
|
|
await heartbeat.resumeQueuedRuns();
|
|
const tick = await heartbeat.tickTimers(new Date("2026-07-07T00:10:00Z"));
|
|
|
|
expect(tick).toEqual({ checked: 0, enqueued: 0, skipped: 0 });
|
|
|
|
const [copiedRun] = await db
|
|
.select({ status: heartbeatRuns.status, startedAt: heartbeatRuns.startedAt })
|
|
.from(heartbeatRuns)
|
|
.where(eq(heartbeatRuns.id, runId));
|
|
expect(copiedRun).toMatchObject({
|
|
status: "queued",
|
|
startedAt: null,
|
|
});
|
|
|
|
const runningCount = await db
|
|
.select({ count: sql<number>`count(*)::int` })
|
|
.from(heartbeatRuns)
|
|
.where(eq(heartbeatRuns.status, "running"))
|
|
.then((rows) => rows[0]?.count ?? 0);
|
|
expect(runningCount).toBe(0);
|
|
});
|
|
|
|
it("skips pre-cutoff system wakes but allows user wakes in an armed worktree", async () => {
|
|
const { agentId, issueId } = await insertAgentAndIssue();
|
|
await armWorktreeRunExecution(new Date(Date.now() + 1_000));
|
|
const heartbeat = heartbeatService(db, {
|
|
runtimeEnv: {
|
|
PAPERCLIP_IN_WORKTREE: "true",
|
|
PAPERCLIP_INSTANCE_ID: "test-worktree",
|
|
},
|
|
});
|
|
|
|
const systemRun = await heartbeat.wakeup(agentId, {
|
|
source: "assignment",
|
|
triggerDetail: "system",
|
|
payload: { issueId },
|
|
contextSnapshot: { issueId },
|
|
requestedByActorType: "system",
|
|
});
|
|
expect(systemRun).toBeNull();
|
|
|
|
const skippedWake = await db
|
|
.select({ reason: agentWakeupRequests.reason, payload: agentWakeupRequests.payload })
|
|
.from(agentWakeupRequests)
|
|
.orderBy(sql`${agentWakeupRequests.createdAt} desc`)
|
|
.limit(1)
|
|
.then((rows) => rows[0] ?? null);
|
|
expect(skippedWake).toMatchObject({ reason: "heartbeat.worktree_execution_cutoff" });
|
|
expect(skippedWake?.payload).toMatchObject({
|
|
heartbeatSkip: { reason: "worktree_execution_cutoff", issueId },
|
|
});
|
|
|
|
const userRun = await heartbeat.wakeup(agentId, {
|
|
source: "on_demand",
|
|
triggerDetail: "user",
|
|
payload: { issueId },
|
|
contextSnapshot: { issueId, skipIssueComment: true },
|
|
requestedByActorType: "user",
|
|
requestedByActorId: "operator",
|
|
});
|
|
expect(userRun).not.toBeNull();
|
|
await heartbeat.waitForRunExecutionDrain(userRun!.id);
|
|
}, 10_000);
|
|
|
|
it("still creates live-plane assignment runs when suppression is not active", async () => {
|
|
const { agentId, issueId } = await insertAgentAndIssue();
|
|
await db
|
|
.update(issues)
|
|
.set({ status: "in_review", updatedAt: new Date() })
|
|
.where(eq(issues.id, issueId));
|
|
|
|
const heartbeat = heartbeatService(db, { runtimeEnv: {} });
|
|
|
|
const run = await heartbeat.wakeup(agentId, {
|
|
source: "assignment",
|
|
triggerDetail: "system",
|
|
reason: "issue_assigned",
|
|
payload: { issueId },
|
|
contextSnapshot: { issueId, wakeReason: "issue_assigned", skipIssueComment: true },
|
|
requestedByActorType: "system",
|
|
requestedByActorId: "issue_assignment",
|
|
});
|
|
|
|
expect(run).not.toBeNull();
|
|
const terminalStatus = await waitForCompletedRun(run!.id, agentId);
|
|
await heartbeat.waitForRunExecutionDrain(run!.id);
|
|
expect(terminalStatus).toBe("succeeded");
|
|
|
|
const runs = await db
|
|
.select({ contextSnapshot: heartbeatRuns.contextSnapshot })
|
|
.from(heartbeatRuns)
|
|
.orderBy(heartbeatRuns.createdAt);
|
|
expect(runs.map((entry) => entry.contextSnapshot?.wakeReason)).toEqual([
|
|
"issue_assigned",
|
|
"issue_review_path_lost",
|
|
]);
|
|
}, 10_000);
|
|
|
|
it("recognizes explicit restore-in-progress suppression", () => {
|
|
expect(resolveHeartbeatSchedulingSuppression({
|
|
PAPERCLIP_DATABASE_RESTORE_IN_PROGRESS: "true",
|
|
})).toEqual({
|
|
suppressed: true,
|
|
reason: "database_restore_in_progress",
|
|
});
|
|
});
|
|
});
|