mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-22 16:51:28 +02:00
Harden productivity review request depth
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
9465d04755
commit
14a149c79f
6 files changed
+206
-30
No files matched your search
@@ -1,4 +1,5 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { MAX_ISSUE_REQUEST_DEPTH } from "../index.js";
|
||||
import {
|
||||
addIssueCommentSchema,
|
||||
createIssueSchema,
|
||||
@@ -75,4 +76,21 @@ describe("issue validators", () => {
|
||||
expect(response.summaryMarkdown).toBe("Summary\n\nNext action");
|
||||
expect(document.body).toBe("# Plan\n\nShip it");
|
||||
});
|
||||
|
||||
it("clamps oversized requestDepth values on create", () => {
|
||||
const parsed = createIssueSchema.parse({
|
||||
title: "Clamp request depth",
|
||||
requestDepth: MAX_ISSUE_REQUEST_DEPTH + 500,
|
||||
});
|
||||
|
||||
expect(parsed.requestDepth).toBe(MAX_ISSUE_REQUEST_DEPTH);
|
||||
});
|
||||
|
||||
it("clamps oversized requestDepth values on update", () => {
|
||||
const parsed = updateIssueSchema.parse({
|
||||
requestDepth: MAX_ISSUE_REQUEST_DEPTH + 1,
|
||||
});
|
||||
|
||||
expect(parsed.requestDepth).toBe(MAX_ISSUE_REQUEST_DEPTH);
|
||||
});
|
||||
});
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
ISSUE_EXECUTION_STAGE_TYPES,
|
||||
ISSUE_EXECUTION_STATE_STATUSES,
|
||||
ISSUE_PRIORITIES,
|
||||
clampIssueRequestDepth,
|
||||
ISSUE_STATUSES,
|
||||
ISSUE_THREAD_INTERACTION_CONTINUATION_POLICIES,
|
||||
ISSUE_THREAD_INTERACTION_KINDS,
|
||||
@@ -123,6 +124,12 @@ export const issueExecutionStateSchema = z.object({
|
||||
lastDecisionOutcome: z.enum(ISSUE_EXECUTION_DECISION_OUTCOMES).nullable(),
|
||||
});
|
||||
|
||||
const issueRequestDepthInputSchema = z
|
||||
.number()
|
||||
.int()
|
||||
.nonnegative()
|
||||
.transform((value) => clampIssueRequestDepth(value));
|
||||
|
||||
export const createIssueSchema = z.object({
|
||||
projectId: z.string().uuid().optional().nullable(),
|
||||
projectWorkspaceId: z.string().uuid().optional().nullable(),
|
||||
@@ -136,7 +143,7 @@ export const createIssueSchema = z.object({
|
||||
priority: z.enum(ISSUE_PRIORITIES).optional().default("medium"),
|
||||
assigneeAgentId: z.string().uuid().optional().nullable(),
|
||||
assigneeUserId: z.string().optional().nullable(),
|
||||
requestDepth: z.number().int().nonnegative().optional().default(0),
|
||||
requestDepth: issueRequestDepthInputSchema.optional().default(0),
|
||||
billingCode: z.string().optional().nullable(),
|
||||
assigneeAdapterOverrides: issueAssigneeAdapterOverridesSchema.optional().nullable(),
|
||||
executionPolicy: issueExecutionPolicySchema.optional().nullable(),
|
||||
@@ -168,6 +175,7 @@ export const createIssueLabelSchema = z.object({
|
||||
export type CreateIssueLabel = z.infer<typeof createIssueLabelSchema>;
|
||||
|
||||
export const updateIssueSchema = createIssueSchema.partial().extend({
|
||||
requestDepth: issueRequestDepthInputSchema.optional(),
|
||||
assigneeAgentId: z.string().trim().min(1).optional().nullable(),
|
||||
comment: multilineTextSchema.pipe(z.string().min(1)).optional(),
|
||||
reviewRequest: issueReviewRequestSchema.optional().nullable(),
|
||||
|
||||
@@ -24,7 +24,7 @@ import {
|
||||
} from "./helpers/embedded-postgres.js";
|
||||
import { instanceSettingsService } from "../services/instance-settings.ts";
|
||||
import { clampIssueListLimit, ISSUE_LIST_MAX_LIMIT, issueService } from "../services/issues.ts";
|
||||
import { buildProjectMentionHref } from "@paperclipai/shared";
|
||||
import { buildProjectMentionHref, MAX_ISSUE_REQUEST_DEPTH } from "@paperclipai/shared";
|
||||
|
||||
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
|
||||
const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
|
||||
@@ -1451,6 +1451,56 @@ describeEmbeddedPostgres("issueService.create workspace inheritance", () => {
|
||||
}),
|
||||
]);
|
||||
});
|
||||
|
||||
it("clamps helper-created child requestDepth to the safe maximum", async () => {
|
||||
const companyId = randomUUID();
|
||||
const projectId = randomUUID();
|
||||
const goalId = randomUUID();
|
||||
const parentIssueId = randomUUID();
|
||||
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: "Paperclip",
|
||||
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
|
||||
requireBoardApprovalForNewAgents: false,
|
||||
});
|
||||
await instanceSettingsService(db).updateExperimental({ enableIsolatedWorkspaces: false });
|
||||
|
||||
await db.insert(goals).values({
|
||||
id: goalId,
|
||||
companyId,
|
||||
title: "Ship child helpers",
|
||||
level: "task",
|
||||
status: "active",
|
||||
});
|
||||
|
||||
await db.insert(projects).values({
|
||||
id: projectId,
|
||||
companyId,
|
||||
goalId,
|
||||
name: "Workspace project",
|
||||
status: "in_progress",
|
||||
});
|
||||
|
||||
await db.insert(issues).values({
|
||||
id: parentIssueId,
|
||||
companyId,
|
||||
projectId,
|
||||
goalId,
|
||||
title: "Parent issue",
|
||||
status: "in_progress",
|
||||
priority: "medium",
|
||||
requestDepth: MAX_ISSUE_REQUEST_DEPTH,
|
||||
});
|
||||
|
||||
const { issue: child } = await svc.createChild(parentIssueId, {
|
||||
title: "Child helper",
|
||||
status: "todo",
|
||||
requestDepth: MAX_ISSUE_REQUEST_DEPTH + 100,
|
||||
});
|
||||
|
||||
expect(child.requestDepth).toBe(MAX_ISSUE_REQUEST_DEPTH);
|
||||
});
|
||||
});
|
||||
|
||||
describeEmbeddedPostgres("issueService blockers and dependency wake readiness", () => {
|
||||
|
||||
@@ -14,6 +14,7 @@ import {
|
||||
getEmbeddedPostgresTestSupport,
|
||||
startEmbeddedPostgresTestDatabase,
|
||||
} from "./helpers/embedded-postgres.js";
|
||||
import { MAX_ISSUE_REQUEST_DEPTH } from "@paperclipai/shared";
|
||||
import {
|
||||
DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS,
|
||||
PRODUCTIVITY_REVIEW_ORIGIN_KIND,
|
||||
@@ -243,6 +244,44 @@ describeEmbeddedPostgres("productivity review service", () => {
|
||||
expect(review?.description).toContain("Runs in rolling windows: 10/1h");
|
||||
});
|
||||
|
||||
it("ignores non-assignee comments when evaluating high-churn productivity reviews", async () => {
|
||||
const now = new Date("2026-04-28T12:00:00.000Z");
|
||||
const seeded = await seedAssignedIssue();
|
||||
await insertRuns({
|
||||
companyId: seeded.companyId,
|
||||
agentId: seeded.coderId,
|
||||
issueId: seeded.issueId,
|
||||
count: 9,
|
||||
now,
|
||||
});
|
||||
const managerRuns = await insertRuns({
|
||||
companyId: seeded.companyId,
|
||||
agentId: seeded.managerId,
|
||||
issueId: seeded.issueId,
|
||||
count: 10,
|
||||
now,
|
||||
});
|
||||
await db.insert(issueComments).values(
|
||||
managerRuns.map((run, index) => ({
|
||||
companyId: seeded.companyId,
|
||||
issueId: seeded.issueId,
|
||||
authorAgentId: seeded.managerId,
|
||||
createdByRunId: run.id,
|
||||
body: `Manager note ${index}`,
|
||||
createdAt: run.createdAt as Date,
|
||||
updatedAt: run.createdAt as Date,
|
||||
})),
|
||||
);
|
||||
|
||||
const result = await productivityReviewService(db).reconcileProductivityReviews({
|
||||
now,
|
||||
companyId: seeded.companyId,
|
||||
});
|
||||
|
||||
expect(result.created).toBe(0);
|
||||
expect(await listProductivityReviews(seeded.companyId)).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("skips productivity-review descendants so reviews cannot recursively spawn reviews", async () => {
|
||||
const now = new Date("2026-04-28T12:00:00.000Z");
|
||||
const seeded = await seedAssignedIssue();
|
||||
@@ -358,4 +397,31 @@ describeEmbeddedPostgres("productivity review service", () => {
|
||||
expect(activities).toHaveLength(1);
|
||||
expect(activities[0]?.entityId).toBe(seeded.issueId);
|
||||
});
|
||||
|
||||
it("clamps poisoned requestDepth metadata instead of aborting productivity reconciliation", async () => {
|
||||
const now = new Date("2026-04-28T12:00:00.000Z");
|
||||
const seeded = await seedAssignedIssue();
|
||||
|
||||
await db
|
||||
.update(issues)
|
||||
.set({ requestDepth: 2_147_483_647 })
|
||||
.where(eq(issues.id, seeded.issueId));
|
||||
|
||||
await insertRuns({
|
||||
companyId: seeded.companyId,
|
||||
agentId: seeded.coderId,
|
||||
issueId: seeded.issueId,
|
||||
count: DEFAULT_PRODUCTIVITY_REVIEW_NO_COMMENT_STREAK_RUNS,
|
||||
now,
|
||||
});
|
||||
|
||||
const result = await productivityReviewService(db).reconcileProductivityReviews({
|
||||
now,
|
||||
companyId: seeded.companyId,
|
||||
});
|
||||
|
||||
expect(result.failed).toBe(0);
|
||||
const [review] = await listProductivityReviews(seeded.companyId);
|
||||
expect(review?.requestDepth).toBe(MAX_ISSUE_REQUEST_DEPTH);
|
||||
});
|
||||
});
|
||||
+2
-2
@@ -707,7 +707,7 @@ export async function startServer(): Promise<StartedServer> {
|
||||
})
|
||||
.then(async () => {
|
||||
const reviewed = await heartbeat.reconcileProductivityReviews();
|
||||
if (reviewed.created > 0 || reviewed.updated > 0) {
|
||||
if (reviewed.created > 0 || reviewed.updated > 0 || reviewed.failed > 0) {
|
||||
logger.warn({ ...reviewed }, "startup productivity reconciliation created or updated review work");
|
||||
}
|
||||
})
|
||||
@@ -772,7 +772,7 @@ export async function startServer(): Promise<StartedServer> {
|
||||
})
|
||||
.then(async () => {
|
||||
const reviewed = await heartbeat.reconcileProductivityReviews();
|
||||
if (reviewed.created > 0 || reviewed.updated > 0) {
|
||||
if (reviewed.created > 0 || reviewed.updated > 0 || reviewed.failed > 0) {
|
||||
logger.warn({ ...reviewed }, "periodic productivity reconciliation created or updated review work");
|
||||
}
|
||||
})
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { and, asc, desc, eq, gt, inArray, isNull, notInArray, sql } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { clampIssueRequestDepth } from "@paperclipai/shared";
|
||||
import {
|
||||
agents,
|
||||
companies,
|
||||
@@ -9,6 +10,7 @@ import {
|
||||
issues,
|
||||
projects,
|
||||
} from "@paperclipai/db";
|
||||
import { logger } from "../middleware/logger.js";
|
||||
import { logActivity } from "./activity-log.js";
|
||||
import { budgetService } from "./budgets.js";
|
||||
import { issueService } from "./issues.js";
|
||||
@@ -262,14 +264,19 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
.then((rows) => rows[0]?.count ?? 0);
|
||||
}
|
||||
|
||||
async function countIssueCommentsSince(companyId: string, issueId: string, since?: Date) {
|
||||
async function countIssueCommentsSince(companyId: string, issueId: string, agentId: string, since?: Date) {
|
||||
return db
|
||||
.select({ count: sql<number>`count(*)::int` })
|
||||
.from(issueComments)
|
||||
.innerJoin(heartbeatRuns, eq(heartbeatRuns.id, issueComments.createdByRunId))
|
||||
.where(
|
||||
and(
|
||||
eq(issueComments.companyId, companyId),
|
||||
eq(issueComments.issueId, issueId),
|
||||
eq(issueComments.authorAgentId, agentId),
|
||||
eq(heartbeatRuns.companyId, companyId),
|
||||
eq(heartbeatRuns.agentId, agentId),
|
||||
issueRunScopeSql(issueId),
|
||||
since ? sql`${issueComments.createdAt} >= ${since.toISOString()}::timestamptz` : undefined,
|
||||
),
|
||||
)
|
||||
@@ -328,23 +335,34 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
const [
|
||||
runCountLastHour,
|
||||
runCountLastSixHours,
|
||||
commentCount,
|
||||
commentCountLastHour,
|
||||
commentCountLastSixHours,
|
||||
assigneeRunCommentCount,
|
||||
assigneeRunCommentCountLastHour,
|
||||
assigneeRunCommentCountLastSixHours,
|
||||
latestComments,
|
||||
costRow,
|
||||
] = await Promise.all([
|
||||
countIssueRunsSince(sourceIssue.companyId, sourceAgent.id, sourceIssue.id, oneHourAgo),
|
||||
countIssueRunsSince(sourceIssue.companyId, sourceAgent.id, sourceIssue.id, sixHoursAgo),
|
||||
countIssueCommentsSince(sourceIssue.companyId, sourceIssue.id),
|
||||
countIssueCommentsSince(sourceIssue.companyId, sourceIssue.id, oneHourAgo),
|
||||
countIssueCommentsSince(sourceIssue.companyId, sourceIssue.id, sixHoursAgo),
|
||||
countIssueCommentsSince(sourceIssue.companyId, sourceIssue.id, sourceAgent.id),
|
||||
countIssueCommentsSince(sourceIssue.companyId, sourceIssue.id, sourceAgent.id, oneHourAgo),
|
||||
countIssueCommentsSince(sourceIssue.companyId, sourceIssue.id, sourceAgent.id, sixHoursAgo),
|
||||
db
|
||||
.select()
|
||||
.select({ comment: issueComments })
|
||||
.from(issueComments)
|
||||
.where(and(eq(issueComments.companyId, sourceIssue.companyId), eq(issueComments.issueId, sourceIssue.id)))
|
||||
.innerJoin(heartbeatRuns, eq(heartbeatRuns.id, issueComments.createdByRunId))
|
||||
.where(
|
||||
and(
|
||||
eq(issueComments.companyId, sourceIssue.companyId),
|
||||
eq(issueComments.issueId, sourceIssue.id),
|
||||
eq(issueComments.authorAgentId, sourceAgent.id),
|
||||
eq(heartbeatRuns.companyId, sourceIssue.companyId),
|
||||
eq(heartbeatRuns.agentId, sourceAgent.id),
|
||||
issueRunScopeSql(sourceIssue.id),
|
||||
),
|
||||
)
|
||||
.orderBy(desc(issueComments.createdAt), desc(issueComments.id))
|
||||
.limit(5),
|
||||
.limit(5)
|
||||
.then((rows) => rows.map((row) => row.comment)),
|
||||
db
|
||||
.select({ costCents: sql<number>`coalesce(sum(${costEvents.costCents}), 0)::int` })
|
||||
.from(costEvents)
|
||||
@@ -364,9 +382,9 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
const longActive = elapsedMs !== null && elapsedMs >= thresholds.longActiveMs;
|
||||
const highChurn =
|
||||
runCountLastHour >= thresholds.highChurnHourly ||
|
||||
commentCountLastHour >= thresholds.highChurnHourly ||
|
||||
assigneeRunCommentCountLastHour >= thresholds.highChurnHourly ||
|
||||
runCountLastSixHours >= thresholds.highChurnSixHours ||
|
||||
commentCountLastSixHours >= thresholds.highChurnSixHours;
|
||||
assigneeRunCommentCountLastSixHours >= thresholds.highChurnSixHours;
|
||||
const trigger = choosePrimaryTrigger({ noComment, longActive, highChurn });
|
||||
if (!trigger) return null;
|
||||
|
||||
@@ -375,7 +393,7 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
if (longActive) triggerReasons.push(`current active episode has lasted ${msToHuman(elapsedMs)}`);
|
||||
if (highChurn) {
|
||||
triggerReasons.push(
|
||||
`${runCountLastHour} runs/${commentCountLastHour} comments in 1h; ${runCountLastSixHours} runs/${commentCountLastSixHours} comments in 6h`,
|
||||
`${runCountLastHour} runs/${assigneeRunCommentCountLastHour} assignee-run comments in 1h; ${runCountLastSixHours} runs/${assigneeRunCommentCountLastSixHours} assignee-run comments in 6h`,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -390,9 +408,9 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
activeRunCount,
|
||||
runCountLastHour,
|
||||
runCountLastSixHours,
|
||||
commentCount,
|
||||
commentCountLastHour,
|
||||
commentCountLastSixHours,
|
||||
commentCount: assigneeRunCommentCount,
|
||||
commentCountLastHour: assigneeRunCommentCountLastHour,
|
||||
commentCountLastSixHours: assigneeRunCommentCountLastSixHours,
|
||||
elapsedMs,
|
||||
latestRuns: latestRuns.slice(0, 5),
|
||||
latestComments,
|
||||
@@ -474,7 +492,7 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
`- No-comment completed-run streak: ${evidence.noCommentStreak}`,
|
||||
`- Current active elapsed time: ${msToHuman(evidence.elapsedMs)}`,
|
||||
`- Runs in rolling windows: ${evidence.runCountLastHour}/1h, ${evidence.runCountLastSixHours}/6h`,
|
||||
`- Comments total/window: ${evidence.commentCount} total, ${evidence.commentCountLastHour}/1h, ${evidence.commentCountLastSixHours}/6h`,
|
||||
`- Assignee run-linked comments total/window: ${evidence.commentCount} total, ${evidence.commentCountLastHour}/1h, ${evidence.commentCountLastSixHours}/6h`,
|
||||
`- Cost events total: ${evidence.costCents} cents`,
|
||||
`- Current next action: ${evidence.nextAction ? truncateInline(evidence.nextAction, 500) : "none recorded"}`,
|
||||
"",
|
||||
@@ -482,14 +500,14 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
"",
|
||||
`- No-comment streak: ${evidence.thresholds.noCommentStreakRuns} completed runs`,
|
||||
`- Long active duration: ${msToHuman(evidence.thresholds.longActiveMs)}`,
|
||||
`- High churn: ${evidence.thresholds.highChurnHourly}/1h or ${evidence.thresholds.highChurnSixHours}/6h runs/comments`,
|
||||
`- High churn: ${evidence.thresholds.highChurnHourly}/1h or ${evidence.thresholds.highChurnSixHours}/6h runs/assignee-run comments`,
|
||||
`- Resolved-review snooze: ${msToHuman(evidence.thresholds.resolvedSnoozeMs)}`,
|
||||
"",
|
||||
"## Latest Runs",
|
||||
"",
|
||||
latestRuns,
|
||||
"",
|
||||
"## Latest Comments",
|
||||
"## Latest Assignee Run Comments",
|
||||
"",
|
||||
latestComments,
|
||||
"",
|
||||
@@ -513,7 +531,7 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
`- Trigger: \`${evidence.trigger}\` (${formatTrigger(evidence.trigger)})`,
|
||||
`- Reasons: ${evidence.triggerReasons.join("; ")}`,
|
||||
`- No-comment streak: ${evidence.noCommentStreak}`,
|
||||
`- Runs/comments: ${evidence.runCountLastHour}/${evidence.commentCountLastHour} in 1h, ${evidence.runCountLastSixHours}/${evidence.commentCountLastSixHours} in 6h`,
|
||||
`- Runs/assignee comments: ${evidence.runCountLastHour}/${evidence.commentCountLastHour} in 1h, ${evidence.runCountLastSixHours}/${evidence.commentCountLastSixHours} in 6h`,
|
||||
`- Next action: ${evidence.nextAction ? truncateInline(evidence.nextAction, 300) : "none recorded"}`,
|
||||
].join("\n");
|
||||
}
|
||||
@@ -561,7 +579,7 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
originKind: PRODUCTIVITY_REVIEW_ORIGIN_KIND,
|
||||
originId: evidence.sourceIssue.id,
|
||||
originFingerprint: productivityReviewFingerprint(evidence.sourceIssue.id),
|
||||
requestDepth: evidence.sourceIssue.requestDepth + 1,
|
||||
requestDepth: clampIssueRequestDepth(clampIssueRequestDepth(evidence.sourceIssue.requestDepth) + 1),
|
||||
});
|
||||
} catch (error) {
|
||||
const maybe = error as { code?: string; constraint?: string; message?: string };
|
||||
@@ -650,7 +668,9 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
existing: 0,
|
||||
snoozed: 0,
|
||||
skipped: 0,
|
||||
failed: 0,
|
||||
reviewIssueIds: [] as string[],
|
||||
failedIssueIds: [] as string[],
|
||||
};
|
||||
|
||||
const prefixCache = new Map<string, string>();
|
||||
@@ -682,11 +702,25 @@ export function productivityReviewService(db: Db, deps?: { enqueueWakeup?: Enque
|
||||
prefix = await getCompanyIssuePrefix(candidate.companyId);
|
||||
prefixCache.set(candidate.companyId, prefix);
|
||||
}
|
||||
const outcome = await createOrUpdateReview(evidence, { prefix });
|
||||
if (outcome.kind === "created") result.created += 1;
|
||||
else if (outcome.kind === "updated") result.updated += 1;
|
||||
else result.existing += 1;
|
||||
result.reviewIssueIds.push(outcome.reviewIssueId);
|
||||
try {
|
||||
const outcome = await createOrUpdateReview(evidence, { prefix });
|
||||
if (outcome.kind === "created") result.created += 1;
|
||||
else if (outcome.kind === "updated") result.updated += 1;
|
||||
else result.existing += 1;
|
||||
result.reviewIssueIds.push(outcome.reviewIssueId);
|
||||
} catch (err) {
|
||||
result.failed += 1;
|
||||
result.failedIssueIds.push(candidate.id);
|
||||
logger.warn(
|
||||
{
|
||||
err,
|
||||
companyId: candidate.companyId,
|
||||
issueId: candidate.id,
|
||||
requestDepth: candidate.requestDepth,
|
||||
},
|
||||
"productivity review reconciliation skipped malformed candidate",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
return result;
|
||||
|
||||
Reference in new issue
Block a user