From fd6cfc7149b014759fb60a76619d643e09ff1f79 Mon Sep 17 00:00:00 2001 From: dotta Date: Thu, 2 Apr 2026 12:09:02 -0500 Subject: [PATCH] fix(routines): address Greptile review findings Co-Authored-By: Paperclip --- cli/src/commands/routines.ts | 8 +++- packages/shared/src/validators/issue.ts | 20 ++++---- packages/shared/src/validators/routine.ts | 14 ++---- server/src/__tests__/routines-service.test.ts | 48 +++++++++++++++++++ server/src/services/routines.ts | 8 +++- 5 files changed, 77 insertions(+), 21 deletions(-) diff --git a/cli/src/commands/routines.ts b/cli/src/commands/routines.ts index 348f7b00..4c5a3ca9 100644 --- a/cli/src/commands/routines.ts +++ b/cli/src/commands/routines.ts @@ -197,10 +197,14 @@ async function openConfiguredDb(configPath: string): Promise<{ await ensurePostgresDatabase(adminConnectionString, "paperclip"); const connectionString = `postgres://paperclip:paperclip@127.0.0.1:${embeddedHandle.port}/paperclip`; await applyPendingMigrations(connectionString); + const db = createDb(connectionString) as ClosableDb; return { - db: createDb(connectionString) as ClosableDb, + db, stop: async () => { - await closeDb(createDb(connectionString) as ClosableDb); + await closeDb(db); + if (embeddedHandle?.startedByThisProcess) { + await embeddedHandle.stop().catch(() => undefined); + } }, }; } diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index a5e3d2a2..17e3b30e 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -1,6 +1,15 @@ import { z } from "zod"; import { ISSUE_PRIORITIES, ISSUE_STATUSES } from "../constants.js"; +export const ISSUE_EXECUTION_WORKSPACE_PREFERENCES = [ + "inherit", + "shared_workspace", + "isolated_workspace", + "operator_branch", + "reuse_existing", + "agent_default", +] as const; + const executionWorkspaceStrategySchema = z .object({ type: z.enum(["project_primary", "git_worktree", "adapter_managed", "cloud_sandbox"]).optional(), @@ -14,7 +23,7 @@ const executionWorkspaceStrategySchema = z export const issueExecutionWorkspaceSettingsSchema = z .object({ - mode: z.enum(["inherit", "shared_workspace", "isolated_workspace", "operator_branch", "reuse_existing", "agent_default"]).optional(), + mode: z.enum(ISSUE_EXECUTION_WORKSPACE_PREFERENCES).optional(), workspaceStrategy: executionWorkspaceStrategySchema.optional().nullable(), workspaceRuntime: z.record(z.unknown()).optional().nullable(), }) @@ -43,14 +52,7 @@ export const createIssueSchema = z.object({ billingCode: z.string().optional().nullable(), assigneeAdapterOverrides: issueAssigneeAdapterOverridesSchema.optional().nullable(), executionWorkspaceId: z.string().uuid().optional().nullable(), - executionWorkspacePreference: z.enum([ - "inherit", - "shared_workspace", - "isolated_workspace", - "operator_branch", - "reuse_existing", - "agent_default", - ]).optional().nullable(), + executionWorkspacePreference: z.enum(ISSUE_EXECUTION_WORKSPACE_PREFERENCES).optional().nullable(), executionWorkspaceSettings: issueExecutionWorkspaceSettingsSchema.optional().nullable(), labelIds: z.array(z.string().uuid()).optional(), }); diff --git a/packages/shared/src/validators/routine.ts b/packages/shared/src/validators/routine.ts index 0bce7722..308ba0ce 100644 --- a/packages/shared/src/validators/routine.ts +++ b/packages/shared/src/validators/routine.ts @@ -7,7 +7,10 @@ import { ROUTINE_TRIGGER_SIGNING_MODES, ROUTINE_VARIABLE_TYPES, } from "../constants.js"; -import { issueExecutionWorkspaceSettingsSchema } from "./issue.js"; +import { + ISSUE_EXECUTION_WORKSPACE_PREFERENCES, + issueExecutionWorkspaceSettingsSchema, +} from "./issue.js"; const routineVariableValueSchema = z.union([z.string(), z.number().finite(), z.boolean()]); @@ -104,14 +107,7 @@ export const runRoutineSchema = z.object({ idempotencyKey: z.string().trim().max(255).optional().nullable(), source: z.enum(["manual", "api"]).optional().default("manual"), executionWorkspaceId: z.string().uuid().optional().nullable(), - executionWorkspacePreference: z.enum([ - "inherit", - "shared_workspace", - "isolated_workspace", - "operator_branch", - "reuse_existing", - "agent_default", - ]).optional().nullable(), + executionWorkspacePreference: z.enum(ISSUE_EXECUTION_WORKSPACE_PREFERENCES).optional().nullable(), executionWorkspaceSettings: issueExecutionWorkspaceSettingsSchema.optional().nullable(), }); diff --git a/server/src/__tests__/routines-service.test.ts b/server/src/__tests__/routines-service.test.ts index 9f1f7ae9..8eee43ce 100644 --- a/server/src/__tests__/routines-service.test.ts +++ b/server/src/__tests__/routines-service.test.ts @@ -466,6 +466,54 @@ describeEmbeddedPostgres("routine service live-execution coalescing", () => { ).rejects.toThrow(/require defaults for required variables/i); }); + it("treats malformed stored defaults as missing when validating schedule triggers", async () => { + const { companyId, agentId, projectId, svc } = await seedFixture(); + const variableRoutine = await svc.create( + companyId, + { + projectId, + goalId: null, + parentIssueId: null, + title: "ship check", + description: "Review {{approved}}", + assigneeAgentId: agentId, + priority: "medium", + status: "active", + concurrencyPolicy: "coalesce_if_active", + catchUpPolicy: "skip_missed", + variables: [ + { name: "approved", label: null, type: "boolean", defaultValue: true, required: true, options: [] }, + ], + }, + {}, + ); + + await db + .update(routines) + .set({ + variables: [ + { + name: "approved", + label: null, + type: "boolean", + defaultValue: "definitely", + required: true, + options: [], + }, + ], + }) + .where(eq(routines.id, variableRoutine.id)); + + await expect( + svc.createTrigger(variableRoutine.id, { + kind: "schedule", + label: "daily", + cronExpression: "0 10 * * *", + timezone: "UTC", + }, {}), + ).rejects.toThrow(/require defaults for required variables/i); + }); + it("serializes concurrent dispatches until the first execution issue is linked to a queued run", async () => { const { routine, svc } = await seedFixture({ wakeup: async (wakeupAgentId, wakeupOpts) => { diff --git a/server/src/services/routines.ts b/server/src/services/routines.ts index 6367a29b..323462e0 100644 --- a/server/src/services/routines.ts +++ b/server/src/services/routines.ts @@ -213,7 +213,13 @@ function sanitizeRoutineVariableInputs( function assertScheduleCompatibleVariables(variables: RoutineVariable[]) { const missingDefaults = variables .filter((variable) => variable.required) - .filter((variable) => isMissingRoutineVariableValue(normalizeRoutineVariableValue(variable, variable.defaultValue))) + .filter((variable) => { + try { + return isMissingRoutineVariableValue(normalizeRoutineVariableValue(variable, variable.defaultValue)); + } catch { + return true; + } + }) .map((variable) => variable.name); if (missingDefaults.length > 0) { throw unprocessable(