diff --git a/app/islands/AutomationForm.tsx b/app/islands/AutomationForm.tsx index 9bae344..b2639fc 100644 --- a/app/islands/AutomationForm.tsx +++ b/app/islands/AutomationForm.tsx @@ -743,8 +743,12 @@ function toVariableDrafts(variables: Variable[] | undefined): VariableDraft[] { })); } -function variableIsComplete(v: VariableDraft): boolean { - return !!v.name.trim() && !!v.value.trim(); +/** Variables are included in the save payload whenever they have a name — even + * if the value is blank. The server will return a `Variable "X" requires a + * value` error, which is the targeted cue duplicators need: the duplicate + * flow intentionally blanks values so the user must supply their own. */ +function variableIsSendable(v: VariableDraft): boolean { + return !!v.name.trim(); } function variableToPayload(v: VariableDraft) { @@ -1225,7 +1229,7 @@ export default function AutomationForm({ if (filteredFetches.length > 0 || isEdit) { payload.fetches = filteredFetches.map((f) => fetchToPayload(f)); } - const filteredVariables = variables.filter((v) => variableIsComplete(v)); + const filteredVariables = variables.filter((v) => variableIsSendable(v)); if (filteredVariables.length > 0 || isEdit) { payload.variables = filteredVariables.map((v) => variableToPayload(v)); } diff --git a/app/routes/api/automations/[rkey].test.ts b/app/routes/api/automations/[rkey].test.ts index 296f5a4..057da5a 100644 --- a/app/routes/api/automations/[rkey].test.ts +++ b/app/routes/api/automations/[rkey].test.ts @@ -351,6 +351,68 @@ describe("PATCH /api/automations/:rkey", () => { const res = await app.request(patchReq("rk1", { actions: [] })); expect(res.status).toBe(400); }); + + describe("variables cross-validation", () => { + it("rejects a PATCH that adds a variable colliding with an existing fetch name", async () => { + // Seed an automation with a fetch named "foo". + await db + .update(automations) + .set({ fetches: [{ kind: "record", name: "foo", uri: "at://did/col/rk" }] }) + .where(eq(automations.uri, TEST_AUTO.uri)); + + // PATCH adds a variable also named "foo" without resending fetches. + const res = await app.request(patchReq("rk1", { variables: [{ name: "foo", value: "x" }] })); + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.error).toMatch(/collide/i); + }); + + it("rejects a PATCH that removes a variable still referenced by an existing action", async () => { + // Seed: a variable `targetCol` referenced by a record action template. + await db + .update(automations) + .set({ + variables: [{ name: "targetCol", value: "app.bsky.feed.post" }], + actions: [ + { + $type: "record", + targetCollection: "app.bsky.feed.post", + recordTemplate: '{"link":"{{targetCol}}","createdAt":"{{now}}"}', + }, + ], + }) + .where(eq(automations.uri, TEST_AUTO.uri)); + + // PATCH clears variables but leaves actions untouched. + const res = await app.request(patchReq("rk1", { variables: [] })); + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.error).toMatch(/unknown placeholder.*targetCol/); + }); + + it("rejects a PATCH that removes a variable still referenced by an existing fetch URI", async () => { + await db + .update(automations) + .set({ + variables: [{ name: "campaignUri", value: "at://did:plc:other/x/y" }], + fetches: [{ kind: "record", name: "target", uri: "{{campaignUri}}" }], + }) + .where(eq(automations.uri, TEST_AUTO.uri)); + + const res = await app.request(patchReq("rk1", { variables: [] })); + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.error).toMatch(/unknown placeholder.*campaignUri/); + }); + + it("accepts a variable-only PATCH when no existing template references it", async () => { + // Seed has no templates referencing any variable. + const res = await app.request( + patchReq("rk1", { variables: [{ name: "anything", value: "v" }] }), + ); + expect(res.status).toBe(200); + }); + }); }); describe("DELETE /api/automations/:rkey", () => { diff --git a/app/routes/api/automations/[rkey].ts b/app/routes/api/automations/[rkey].ts index 1767ce7..a69a0b9 100644 --- a/app/routes/api/automations/[rkey].ts +++ b/app/routes/api/automations/[rkey].ts @@ -2,7 +2,7 @@ import { createRoute } from "honox/factory"; import { eq, and, desc } from "drizzle-orm"; import { db } from "@/db/index.js"; import { automations, deliveryLogs, type Action } from "@/db/schema.js"; -import { ACTION_REGISTRY } from "@/actions/registry.js"; +import { ACTION_REGISTRY, isRecordProducingAction } from "@/actions/registry.js"; import { config } from "@/config.js"; import { getRecord, putRecord, deleteRecord, type PdsAction } from "@/automations/pds.js"; import { toPdsAction, toPdsFetch, toPdsVariable } from "@/automations/pds-serialize.js"; @@ -18,6 +18,8 @@ import { normalizeFetches, normalizeConditions, normalizeVariables, + checkVariableFetchCollision, + checkExistingTemplateReferences, type FetchInput, type ConditionInput, type VariableInput, @@ -252,6 +254,28 @@ export const PATCH = createRoute(async (c) => { pdsActions = newPdsActions; } + // Cross-checks against the final variable/fetch/action state. POST enforces + // both invariants implicitly because every input flows through the per-step + // validators in one go; PATCH can omit one side of the body, so we re-check + // here against the kept-as-is portions. + const collision = checkVariableFetchCollision(localVariables, localFetches); + if (!collision.ok) return c.json({ error: collision.error }, 400); + + // Only walk existing templates when the variable set actually changed — + // otherwise nothing new can have invalidated a reference. + if (body.variables) { + const actionResultNames = localActions + .map((a, i) => (isRecordProducingAction(a.$type) ? `action${i + 1}` : null)) + .filter((name): name is string => name !== null); + const refs = checkExistingTemplateReferences( + localActions, + localFetches, + variableNames, + actionResultNames, + ); + if (!refs.ok) return c.json({ error: refs.error }, 400); + } + // Re-verify webhook callbacks when reactivating (updates verified status). // Webhook is the only action with a callback to re-check; the inline $type // narrowing is intentional rather than a missed dispatcher migration — diff --git a/app/routes/api/automations/index.test.ts b/app/routes/api/automations/index.test.ts index c34c6f8..2a02533 100644 --- a/app/routes/api/automations/index.test.ts +++ b/app/routes/api/automations/index.test.ts @@ -583,6 +583,92 @@ describe("POST /api/automations", () => { expect(body.error).toMatch(/literal/); }); + it("accepts a variable referenced from a secondary action template field (bsky-post embedExternalUrl)", async () => { + // Regression: secondary template fields previously called validateTextTemplate + // without `variableNames`, rejecting valid uses at save time even though + // the runtime renderer happily resolves them. + const res = await app.request( + jsonReq("/api/automations", { + name: "Var in embed url", + lexicon: "app.bsky.feed.like", + operations: ["create"], + variables: [{ name: "linkUrl", value: "https://example.com/post" }], + actions: [ + { + type: "bsky-post", + textTemplate: "hello", + embedExternalUrl: "{{linkUrl}}", + }, + ], + }), + ); + expect(res.status).toBe(201); + }); + + it("accepts a variable referenced from margin-bookmark bodyValue / tags", async () => { + const res = await app.request( + jsonReq("/api/automations", { + name: "Var in margin", + lexicon: "app.bsky.feed.like", + operations: ["create"], + variables: [ + { name: "snippet", value: "Saved automatically" }, + { name: "tagName", value: "auto" }, + ], + actions: [ + { + type: "margin-bookmark", + targetSource: "https://example.com", + bodyValue: "{{snippet}}", + tags: ["{{tagName}}"], + }, + ], + }), + ); + expect(res.status).toBe(201); + }); + + it("accepts a variable referenced from kipclip-annotation url / note", async () => { + const res = await app.request( + jsonReq("/api/automations", { + name: "Var in kipclip", + lexicon: "app.bsky.feed.like", + operations: ["create"], + variables: [ + { name: "linkUrl", value: "https://example.com" }, + { name: "annot", value: "saved via airglow" }, + ], + actions: [ + { + type: "kipclip-annotation", + subject: "at://did:plc:x/com.kipclip.bookmark/abc", + url: "{{linkUrl}}", + note: "{{annot}}", + }, + ], + }), + ); + expect(res.status).toBe(201); + }); + + it("rejects a blank-value variable with a targeted error (duplicate-flow cue)", async () => { + // Regression: the form previously filtered blank-value variables out + // of the save payload, swallowing this error. The targeted message is + // the cue duplicators need. + const res = await app.request( + jsonReq("/api/automations", { + name: "Blank", + lexicon: "app.bsky.feed.like", + operations: ["create"], + variables: [{ name: "needsValue", value: "" }], + actions: [{ type: "webhook", callbackUrl: "https://example.com/hook" }], + }), + ); + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.error).toMatch(/Variable "needsValue" requires a value/); + }); + it("accepts a variable referenced from an action template", async () => { const res = await app.request( jsonReq("/api/automations", { diff --git a/lib/actions/api-normalize.ts b/lib/actions/api-normalize.ts index 0f8a2e3..c43cf4a 100644 --- a/lib/actions/api-normalize.ts +++ b/lib/actions/api-normalize.ts @@ -1,4 +1,4 @@ -import type { FetchStep, Condition, Variable } from "../db/schema.js"; +import type { Action, FetchStep, Condition, Variable } from "../db/schema.js"; import type { PdsFetchStep } from "../automations/pds.js"; import { validateFetchSearchStep, @@ -8,7 +8,7 @@ import { type FetchSearchInput, type FetchConditionInput, } from "./validation.js"; -import { validateFetchStep, validateVariable } from "./template.js"; +import { validateFetchStep, validateVariable, PLACEHOLDER_RE } from "./template.js"; import { isValidNsid } from "../lexicons/resolver.js"; import { AUTOMATION_LIMITS } from "../automations/limits.js"; import { toPdsFetch } from "../automations/pds-serialize.js"; @@ -185,3 +185,105 @@ export function normalizeVariables( return { ok: true, value, names }; } + +/** Reject the request when a variable name collides with a fetch name. POST + * catches this from both sides naturally (variables normalize first, then + * fetches see the variable names), but a PATCH that touches only one side + * would otherwise let drift through. Call after the final `localVariables` + * and `localFetches` are settled. */ +export function checkVariableFetchCollision( + variables: Variable[], + fetches: FetchStep[], +): Err | { ok: true } { + const fetchNameSet = new Set(fetches.map((f) => f.name)); + for (const v of variables) { + if (fetchNameSet.has(v.name)) { + return { ok: false, error: `Variable name "${v.name}" collides with a fetch name` }; + } + } + return { ok: true }; +} + +const RESERVED_PLACEHOLDER_ROOTS = new Set(["event", "now", "self", "item", "automation"]); + +function collectTemplateStrings(actions: Action[], fetches: FetchStep[]): string[] { + const out: string[] = []; + for (const f of fetches) { + if (f.kind === "search") { + out.push(f.repo); + for (const w of f.where) out.push(w.value); + } else { + out.push(f.uri); + } + } + for (const a of actions) { + switch (a.$type) { + case "record": + out.push(a.recordTemplate); + break; + case "patch-record": + out.push(a.baseRecordUri, a.recordTemplate); + break; + case "bsky-post": + out.push(a.textTemplate); + if (a.embedExternalUrl) out.push(a.embedExternalUrl); + break; + case "margin-bookmark": + out.push(a.targetSource); + if (a.bodyValue) out.push(a.bodyValue); + if (a.tags) out.push(...a.tags); + break; + case "follow": + out.push(a.subject); + break; + case "semble-save": + out.push(a.url); + break; + case "calendar-rsvp": + out.push(a.subject); + break; + case "kipclip-annotation": + out.push(a.subject); + if (a.url) out.push(a.url); + if (a.note) out.push(a.note); + break; + // webhook callbackUrl is a literal URL, not templated + } + } + return out; +} + +/** Verify every `{{root}}` placeholder in the given templates resolves against + * the current variable / fetch / action-result name spaces. Used by PATCH to + * catch the case where a client updates variables but keeps fetches/actions + * as-is: a removed or renamed variable still referenced by an existing + * template would otherwise persist as silently broken state. */ +export function checkExistingTemplateReferences( + actions: Action[], + fetches: FetchStep[], + variableNames: string[], + actionResultNames: string[], +): Err | { ok: true } { + const variableSet = new Set(variableNames); + const fetchSet = new Set(fetches.map((f) => f.name)); + const actionSet = new Set(actionResultNames); + + for (const template of collectTemplateStrings(actions, fetches)) { + for (const match of template.matchAll(PLACEHOLDER_RE)) { + const raw = match[1]!.trim(); + // Strip function-call wrappers like didToHandle(event.did) → event.did + const innerArg = raw.match(/^\w+\((.+)\)$/)?.[1]?.trim(); + const path = innerArg ?? raw; + if (variableSet.has(path)) continue; + const root = path.split(".")[0]!; + if (RESERVED_PLACEHOLDER_ROOTS.has(root)) continue; + if (fetchSet.has(root)) continue; + if (actionSet.has(root)) continue; + return { + ok: false, + error: `Existing template references unknown placeholder {{${raw}}} after this change`, + }; + } + } + return { ok: true }; +} diff --git a/lib/actions/bsky-post.ts b/lib/actions/bsky-post.ts index 583a848..f691813 100644 --- a/lib/actions/bsky-post.ts +++ b/lib/actions/bsky-post.ts @@ -238,6 +238,7 @@ async function validate( ctx.fetchNames, ctx.actionResultNames, ctx.hasItem, + ctx.variableNames, ); if (!embedValidation.valid) { return { ok: false, error: `embedExternalUrl: ${embedValidation.error}` }; diff --git a/lib/actions/kipclip-annotation.ts b/lib/actions/kipclip-annotation.ts index 7974a53..38aab05 100644 --- a/lib/actions/kipclip-annotation.ts +++ b/lib/actions/kipclip-annotation.ts @@ -184,6 +184,7 @@ async function validate( ctx.fetchNames, ctx.actionResultNames, ctx.hasItem, + ctx.variableNames, ); if (!urlCheck.valid) { return { ok: false, error: `url: ${urlCheck.error}` }; @@ -209,6 +210,7 @@ async function validate( ctx.fetchNames, ctx.actionResultNames, ctx.hasItem, + ctx.variableNames, ); if (!noteCheck.valid) { return { ok: false, error: `note: ${noteCheck.error}` }; diff --git a/lib/actions/margin-bookmark.ts b/lib/actions/margin-bookmark.ts index 83df97e..32f447a 100644 --- a/lib/actions/margin-bookmark.ts +++ b/lib/actions/margin-bookmark.ts @@ -198,6 +198,7 @@ async function validate( ctx.fetchNames, ctx.actionResultNames, ctx.hasItem, + ctx.variableNames, ); if (!bodyValidation.valid) { return { ok: false, error: `bodyValue: ${bodyValidation.error}` }; @@ -230,6 +231,7 @@ async function validate( ctx.fetchNames, ctx.actionResultNames, ctx.hasItem, + ctx.variableNames, ); if (!tagValidation.valid) { return { ok: false, error: `tag: ${tagValidation.error}` };