From 9093cfbe4ff6fd0c71b502fbaa7c784743a13485 Mon Sep 17 00:00:00 2001 From: dotta Date: Fri, 20 Mar 2026 16:26:29 -0500 Subject: [PATCH] fix: address greptile routine review --- server/src/__tests__/routines-service.test.ts | 45 +++++++++++- server/src/services/routines.ts | 10 ++- ui/src/lib/routine-trigger-patch.test.ts | 71 +++++++++++++++++++ ui/src/lib/routine-trigger-patch.ts | 30 ++++++++ ui/src/pages/RoutineDetail.tsx | 18 +---- ui/src/pages/Routines.tsx | 2 +- 6 files changed, 157 insertions(+), 19 deletions(-) create mode 100644 ui/src/lib/routine-trigger-patch.test.ts create mode 100644 ui/src/lib/routine-trigger-patch.ts diff --git a/server/src/__tests__/routines-service.test.ts b/server/src/__tests__/routines-service.test.ts index 31e487ac..54a56f54 100644 --- a/server/src/__tests__/routines-service.test.ts +++ b/server/src/__tests__/routines-service.test.ts @@ -1,4 +1,4 @@ -import { randomUUID } from "node:crypto"; +import { createHmac, randomUUID } from "node:crypto"; import fs from "node:fs"; import net from "node:net"; import os from "node:os"; @@ -6,9 +6,12 @@ import path from "node:path"; import { eq } from "drizzle-orm"; import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; import { + activityLog, agents, applyPendingMigrations, companies, + companySecrets, + companySecretVersions, createDb, ensurePostgresDatabase, heartbeatRuns, @@ -16,6 +19,7 @@ import { projects, routineRuns, routines, + routineTriggers, } from "@paperclipai/db"; import { issueService } from "../services/issues.ts"; import { routineService } from "../services/routines.ts"; @@ -99,8 +103,12 @@ describe("routine service live-execution coalescing", () => { }, 20_000); afterEach(async () => { + await db.delete(activityLog); await db.delete(routineRuns); + await db.delete(routineTriggers); await db.delete(routines); + await db.delete(companySecretVersions); + await db.delete(companySecrets); await db.delete(heartbeatRuns); await db.delete(issues); await db.delete(projects); @@ -421,4 +429,39 @@ describe("routine service live-execution coalescing", () => { expect(routineIssues).toHaveLength(1); }); + + it("accepts standard second-precision webhook timestamps for HMAC triggers", async () => { + const { routine, svc } = await seedFixture(); + const { trigger, secretMaterial } = await svc.createTrigger( + routine.id, + { + kind: "webhook", + signingMode: "hmac_sha256", + replayWindowSec: 300, + }, + {}, + ); + + expect(trigger.publicId).toBeTruthy(); + expect(secretMaterial?.webhookSecret).toBeTruthy(); + + const payload = { ok: true }; + const rawBody = Buffer.from(JSON.stringify(payload)); + const timestampSeconds = String(Math.floor(Date.now() / 1000)); + const signature = `sha256=${createHmac("sha256", secretMaterial!.webhookSecret) + .update(`${timestampSeconds}.`) + .update(rawBody) + .digest("hex")}`; + + const run = await svc.firePublicTrigger(trigger.publicId!, { + signatureHeader: signature, + timestampHeader: timestampSeconds, + rawBody, + payload, + }); + + expect(run.source).toBe("webhook"); + expect(run.status).toBe("issue_created"); + expect(run.linkedIssueId).toBeTruthy(); + }); }); diff --git a/server/src/services/routines.ts b/server/src/services/routines.ts index 2243c1b1..8eb465f2 100644 --- a/server/src/services/routines.ts +++ b/server/src/services/routines.ts @@ -132,6 +132,12 @@ function nextResultText(status: string, issueId?: string | null) { return status; } +function normalizeWebhookTimestampMs(rawTimestamp: string) { + const parsed = Number(rawTimestamp); + if (!Number.isFinite(parsed)) return null; + return parsed > 1e12 ? parsed : parsed * 1000; +} + export function routineService(db: Db, deps: { heartbeat?: IssueAssignmentWakeupDeps } = {}) { const issueSvc = issueService(db); const secretsSvc = secretService(db); @@ -1064,8 +1070,8 @@ export function routineService(db: Db, deps: { heartbeat?: IssueAssignmentWakeup const providedSignature = input.signatureHeader?.trim() ?? ""; const providedTimestamp = input.timestampHeader?.trim() ?? ""; if (!providedSignature || !providedTimestamp) throw unauthorized(); - const tsMillis = Number(providedTimestamp); - if (!Number.isFinite(tsMillis)) throw unauthorized(); + const tsMillis = normalizeWebhookTimestampMs(providedTimestamp); + if (tsMillis == null) throw unauthorized(); const replayWindowSec = trigger.replayWindowSec ?? 300; if (Math.abs(Date.now() - tsMillis) > replayWindowSec * 1000) { throw unauthorized(); diff --git a/ui/src/lib/routine-trigger-patch.test.ts b/ui/src/lib/routine-trigger-patch.test.ts new file mode 100644 index 00000000..ccaf5e8a --- /dev/null +++ b/ui/src/lib/routine-trigger-patch.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; +import type { RoutineTrigger } from "@paperclipai/shared"; +import { buildRoutineTriggerPatch } from "./routine-trigger-patch"; + +function makeScheduleTrigger(overrides: Partial = {}): RoutineTrigger { + return { + id: "trigger-1", + companyId: "company-1", + routineId: "routine-1", + kind: "schedule", + label: "Daily", + enabled: true, + cronExpression: "0 10 * * *", + timezone: "UTC", + nextRunAt: null, + lastFiredAt: null, + publicId: null, + secretId: null, + signingMode: null, + replayWindowSec: null, + lastRotatedAt: null, + lastResult: null, + createdByAgentId: null, + createdByUserId: null, + updatedByAgentId: null, + updatedByUserId: null, + createdAt: new Date("2026-03-20T00:00:00.000Z"), + updatedAt: new Date("2026-03-20T00:00:00.000Z"), + ...overrides, + }; +} + +describe("buildRoutineTriggerPatch", () => { + it("preserves an existing schedule trigger timezone when saving edits", () => { + const patch = buildRoutineTriggerPatch( + makeScheduleTrigger({ timezone: "UTC" }), + { + label: "Daily label edit", + cronExpression: "0 10 * * *", + signingMode: "bearer", + replayWindowSec: "300", + }, + "America/Chicago", + ); + + expect(patch).toEqual({ + label: "Daily label edit", + cronExpression: "0 10 * * *", + timezone: "UTC", + }); + }); + + it("falls back to the local timezone when a schedule trigger has none", () => { + const patch = buildRoutineTriggerPatch( + makeScheduleTrigger({ timezone: null }), + { + label: "", + cronExpression: "15 9 * * 1-5", + signingMode: "bearer", + replayWindowSec: "300", + }, + "America/Chicago", + ); + + expect(patch).toEqual({ + label: null, + cronExpression: "15 9 * * 1-5", + timezone: "America/Chicago", + }); + }); +}); diff --git a/ui/src/lib/routine-trigger-patch.ts b/ui/src/lib/routine-trigger-patch.ts new file mode 100644 index 00000000..df8b1c92 --- /dev/null +++ b/ui/src/lib/routine-trigger-patch.ts @@ -0,0 +1,30 @@ +import type { RoutineTrigger } from "@paperclipai/shared"; + +export type RoutineTriggerEditorDraft = { + label: string; + cronExpression: string; + signingMode: string; + replayWindowSec: string; +}; + +export function buildRoutineTriggerPatch( + trigger: RoutineTrigger, + draft: RoutineTriggerEditorDraft, + fallbackTimezone: string, +) { + const patch: Record = { + label: draft.label.trim() || null, + }; + + if (trigger.kind === "schedule") { + patch.cronExpression = draft.cronExpression.trim(); + patch.timezone = trigger.timezone ?? fallbackTimezone; + } + + if (trigger.kind === "webhook") { + patch.signingMode = draft.signingMode; + patch.replayWindowSec = Number(draft.replayWindowSec || "300"); + } + + return patch; +} diff --git a/ui/src/pages/RoutineDetail.tsx b/ui/src/pages/RoutineDetail.tsx index 5b520261..f357878d 100644 --- a/ui/src/pages/RoutineDetail.tsx +++ b/ui/src/pages/RoutineDetail.tsx @@ -24,6 +24,7 @@ import { useCompany } from "../context/CompanyContext"; import { useBreadcrumbs } from "../context/BreadcrumbContext"; import { useToast } from "../context/ToastContext"; import { queryKeys } from "../lib/queryKeys"; +import { buildRoutineTriggerPatch } from "../lib/routine-trigger-patch"; import { timeAgo } from "../lib/timeAgo"; import { EmptyState } from "../components/EmptyState"; import { PageSkeleton } from "../components/PageSkeleton"; @@ -61,7 +62,7 @@ const concurrencyPolicyDescriptions: Record = { }; const catchUpPolicyDescriptions: Record = { skip_missed: "Ignore schedule windows that were missed while the routine or scheduler was paused.", - enqueue_missed_with_cap: "Catch up missed schedule windows with a capped backlog after recovery.", + enqueue_missed_with_cap: "Catch up missed schedule windows in capped batches after recovery.", }; const signingModeDescriptions: Record = { bearer: "Expect a shared bearer token in the Authorization header.", @@ -212,20 +213,7 @@ function TriggerEditor({