From f2ee4369c3b96c1eeb9f985c53f9754ad41d084c Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Fri, 25 Sep 2026 15:24:00 +0900 Subject: [PATCH 01/11] Move the shared listener analysis into lib The reachability walk, the name-holding analysis and the outbox listener discovery were written inside outbox-listener-delivery-required, but a rule that checks how a listener's delivery calls are awaited needs the same three pieces. Move them into lib/reachability.ts and lib/outbox-listener.ts so both rules can use them, and leave the delivery text scan and the report in the rule. The moved code is unchanged apart from export keywords, imports, and createRule calling createOutboxListenerVisitor instead of inlining the listener discovery. The existing tests are unchanged and still pass. https://github.com/fedify-dev/fedify/issues/1057 Assisted-by: Claude Code:claude-sonnet-5 --- packages/lint/src/lib/outbox-listener.ts | 199 +++++ packages/lint/src/lib/reachability.ts | 528 +++++++++++++ .../outbox-listener-delivery-required.ts | 713 +----------------- 3 files changed, 744 insertions(+), 696 deletions(-) create mode 100644 packages/lint/src/lib/outbox-listener.ts create mode 100644 packages/lint/src/lib/reachability.ts diff --git a/packages/lint/src/lib/outbox-listener.ts b/packages/lint/src/lib/outbox-listener.ts new file mode 100644 index 000000000..806469c9b --- /dev/null +++ b/packages/lint/src/lib/outbox-listener.ts @@ -0,0 +1,199 @@ +import { + hasIdentifierProperty, + hasMemberExpressionCallee, + hasMethodName, + isFunction, + isNode, +} from "./pred.ts"; +import type { FunctionLikeNode } from "./reachability.ts"; +import { trackFederationVariables } from "./tracker.ts"; +import type { + AssignmentPattern, + CallExpression, + Expression, + Identifier, + Node, + VariableDeclarator, +} from "./types.ts"; + +export const DELIVERY_METHOD_NAMES = new Set([ + "sendActivity", + "forwardActivity", +]); + +const isChainedFromOutboxListeners = ( + expr: Expression, + federationTracker: ReturnType, +): boolean => { + if (expr.type !== "CallExpression") return false; + if (!hasMemberExpressionCallee(expr) || !hasIdentifierProperty(expr)) { + return false; + } + const methodName = expr.callee.property.name; + if (methodName === "setOutboxListeners") { + return federationTracker.isFederationObject(expr.callee.object); + } + if ( + methodName === "authorize" || methodName === "onError" || + methodName === "on" + ) { + return isChainedFromOutboxListeners(expr.callee.object, federationTracker); + } + return false; +}; + +const getMemberPropertyName = (expr: Expression): string | null => { + if (expr.type !== "MemberExpression") return null; + const property = expr.property as Node; + if (property.type === "Identifier") return property.name; + if (property.type === "Literal" && typeof property.value === "string") { + return property.value; + } + return null; +}; + +export function unwrapContextParam(node: Node | undefined): Node | null { + let current: Node | null = node ?? null; + while (current?.type === "AssignmentPattern") { + current = (current as AssignmentPattern).left as Node; + } + return current; +} + +/** + * Resolves an expression to the function it refers to: a direct function + * literal, a local variable bound to one, or a property of a local object + * literal bound to one (e.g. `handlers.deliver` where + * `const handlers = { deliver() {} }`). Used to resolve a listener argument + * (`.on(Activity, handler)`). + */ +const resolveFunctionBinding = ( + expr: Expression, + bindings: Map, + seen = new Set(), +): FunctionLikeNode | null => { + if (isFunction(expr)) return expr; + if (expr.type === "Identifier") { + if (seen.has(expr.name)) return null; + seen.add(expr.name); + const binding = bindings.get(expr.name); + if (binding == null || !isNode(binding)) return null; + if ( + isFunction(binding as Expression) || + (binding as { type?: string }).type === "FunctionDeclaration" + ) { + return binding as FunctionLikeNode; + } + if (binding.type === "Identifier") { + return resolveFunctionBinding(binding, bindings, seen); + } + return null; + } + if ( + expr.type === "MemberExpression" && expr.object.type === "Identifier" && + !expr.computed + ) { + const binding = bindings.get(expr.object.name); + if ( + binding == null || !isNode(binding) || binding.type !== "ObjectExpression" + ) { + return null; + } + const propertyName = getMemberPropertyName(expr); + if (propertyName == null) return null; + for (const prop of binding.properties) { + if (!isNode(prop) || prop.type !== "Property") continue; + const keyName = prop.key.type === "Identifier" + ? prop.key.name + : prop.key.type === "Literal" && typeof prop.key.value === "string" + ? prop.key.value + : null; + if (keyName !== propertyName || !isNode(prop.value)) continue; + const value = prop.value as unknown; + if ( + isFunction(value as Expression) || + (value as { type?: string }).type === "FunctionDeclaration" + ) { + return value as FunctionLikeNode; + } + } + } + return null; +}; + +type OutboxListenerVisitor = { + VariableDeclarator(node: VariableDeclarator): void; + FunctionDeclaration( + node: Node & { type: "FunctionDeclaration"; id: Identifier | null }, + ): void; + CallExpression(node: CallExpression): void; + "Program:exit"(): void; +}; + +/** + * Builds the visitor an outbox-listener rule registers: it follows + * `federation.setOutboxListeners(...).on(Type, listener)` chains through the + * file and, once the whole file has been seen, hands each listener, resolved + * to its function, to `onListener`. The listener may be an inline function or + * a name bound to one elsewhere in the file. + */ +export function createOutboxListenerVisitor( + onListener: (listener: FunctionLikeNode) => void, +): OutboxListenerVisitor { + const federationTracker = trackFederationVariables(); + const bindings = new Map(); + const pendingCalls: CallExpression[] = []; + + const inspectCall = (node: CallExpression): void => { + if ( + !hasMemberExpressionCallee(node) || + !hasIdentifierProperty(node) || + !hasMethodName("on")(node) || + node.arguments.length < 2 + ) { + return; + } + if ( + !isChainedFromOutboxListeners(node.callee.object, federationTracker) + ) { + return; + } + + const listener = node.arguments[1] as unknown; + const resolvedListener = + isNode(listener) && isFunction(listener as Expression) + ? listener as FunctionLikeNode + : isNode(listener) + ? resolveFunctionBinding(listener as Expression, bindings) + : null; + if (resolvedListener == null) return; + + onListener(resolvedListener); + }; + + return { + VariableDeclarator(node: VariableDeclarator): void { + federationTracker.VariableDeclarator(node); + if (node.id.type === "Identifier" && node.init != null) { + bindings.set(node.id.name, node.init); + } + }, + + FunctionDeclaration( + node: Node & { + type: "FunctionDeclaration"; + id: Identifier | null; + }, + ): void { + if (node.id != null) bindings.set(node.id.name, node); + }, + + CallExpression(node: CallExpression): void { + pendingCalls.push(node); + }, + + "Program:exit"(): void { + for (const node of pendingCalls) inspectCall(node); + }, + }; +} diff --git a/packages/lint/src/lib/reachability.ts b/packages/lint/src/lib/reachability.ts new file mode 100644 index 000000000..32d527db3 --- /dev/null +++ b/packages/lint/src/lib/reachability.ts @@ -0,0 +1,528 @@ +import { isNode } from "./pred.ts"; +import type { + Expression, + FunctionNode, + Identifier, + Node, + VariableDeclarator, +} from "./types.ts"; + +export type FunctionLikeNode = + | FunctionNode + | (Node & { + type: "FunctionDeclaration"; + id: Identifier | null; + params: unknown[]; + body: unknown; + }); + +const FUNCTION_NODE_TYPES = new Set([ + "FunctionDeclaration", + "FunctionExpression", + "ArrowFunctionExpression", +]); + +const isFunctionLikeNode = (node: Node): node is FunctionLikeNode => + FUNCTION_NODE_TYPES.has(node.type); + +// --------------------------------------------------------------------------- +// Reachability: which statements can actually run, following control flow +// (if/else, try/catch/finally, switch, loops) but never descending into a +// nested function's own body, and pruning dead code (a statically-falsy `if` +// branch, or anything after a statement that always returns/throws). +// +// A control-flow statement's head expressions (an `if` test, a `switch` +// discriminant and case tests, a loop's `init`/`test`/`update`/`right`) run +// whenever the statement itself does, whichever branch is taken, so they are +// collected alongside the bodies. Collecting one never revives the branch +// behind it: `if (false)` still hides its consequent. +// --------------------------------------------------------------------------- + +const isStaticallyFalsy = (test: Expression): boolean => + test.type === "Literal" && !test.value; + +const isStaticallyTruthy = (test: Expression): boolean => + test.type === "Literal" && Boolean(test.value); + +/** + * Whether every path through this statement unconditionally returns or + * throws, meaning anything textually after it in the same statement list + * never runs. Deliberately conservative: when it can't prove that, it + * answers `false`, which keeps the following code counted as reachable + * (a missed dead-code case is safer than wrongly hiding live code). + */ +function alwaysExits(node: Node): boolean { + switch (node.type) { + case "ReturnStatement": + case "ThrowStatement": + case "BreakStatement": + case "ContinueStatement": + return true; + + case "BlockStatement": + return node.body.some((statement) => alwaysExits(statement as Node)); + + case "IfStatement": { + const test = node.test as Expression; + if (isStaticallyFalsy(test)) { + return node.alternate != null && alwaysExits(node.alternate as Node); + } + if (isStaticallyTruthy(test)) { + return alwaysExits(node.consequent as Node); + } + if (node.alternate == null) return false; + return alwaysExits(node.consequent as Node) && + alwaysExits(node.alternate as Node); + } + + case "TryStatement": + // A `finally` that always exits dominates the whole statement. Beyond + // that, a `try` block can throw partway through and jump to `catch`, + // so proving more than this would need tracking which statements can + // throw -- stay conservative and say "not sure" instead. + return node.finalizer != null && alwaysExits(node.finalizer as Node); + + default: + return false; + } +} + +export function collectReachableStatements(node: Node, out: Node[]): void { + switch (node.type) { + case "BlockStatement": + for (const [index, statement] of node.body.entries()) { + collectReachableStatements(statement as Node, out); + if (alwaysExits(statement as Node)) { + // Function declarations hoist: one written below an exit is still + // callable from the code above it. + for (const rest of node.body.slice(index + 1)) { + if ((rest as Node).type === "FunctionDeclaration") { + out.push(rest as Node); + } + } + return; + } + } + return; + + case "IfStatement": { + const test = node.test as Expression; + out.push(test); + if (!isStaticallyFalsy(test)) { + collectReachableStatements(node.consequent as Node, out); + } + if (node.alternate != null && !isStaticallyTruthy(test)) { + collectReachableStatements(node.alternate as Node, out); + } + return; + } + + case "TryStatement": + collectReachableStatements(node.block as Node, out); + if (node.handler != null) { + collectReachableStatements(node.handler.body as Node, out); + } + if (node.finalizer != null) { + collectReachableStatements(node.finalizer as Node, out); + } + return; + + case "SwitchStatement": + out.push(node.discriminant as Node); + for (const switchCase of node.cases) { + if (switchCase.test != null) out.push(switchCase.test as Node); + for (const statement of switchCase.consequent) { + collectReachableStatements(statement as Node, out); + if (alwaysExits(statement as Node)) break; + } + } + return; + + case "WhileStatement": + case "DoWhileStatement": + out.push(node.test as Node); + collectReachableStatements(node.body as Node, out); + return; + + case "ForStatement": + for (const head of [node.init, node.test, node.update]) { + if (head != null) out.push(head as Node); + } + collectReachableStatements(node.body as Node, out); + return; + + case "ForInStatement": + case "ForOfStatement": + // Only `right` is evaluated as a value; `left` declares or assigns the + // loop variable. + out.push(node.right as Node); + collectReachableStatements(node.body as Node, out); + return; + + case "LabeledStatement": + collectReachableStatements(node.body as Node, out); + return; + + case "WithStatement": + out.push(node.object as Node); + collectReachableStatements(node.body as Node, out); + return; + + default: + out.push(node); + return; + } +} + +// --------------------------------------------------------------------------- +// What a listener (or a helper's own body) resolves to when scanned for a +// delivery call: two rules decide which nested function bodies are folded +// into the scan instead of being masked out. +// +// The rule reports only when neither can account for a delivery call, so +// both err toward treating a function as used. Working out how a function +// value travels through arbitrary JavaScript (an alias, a destructured +// property, an array, a wrapper call) is open-ended, and so is working out +// what a call does with a callback it receives. Showing that a name never +// appears anywhere that runs is not. So a function held under a name is +// used as soon as that name is mentioned, without tracing how it is then +// passed around, and any other function literal, such as a callback handed +// to a call, is used wherever it appears, since the rule cannot show that +// the receiving call never runs it. A missed warning is the safe direction; +// a warning on code that delivers is not. +// --------------------------------------------------------------------------- + +/** + * Collects plain-value references to identifiers: `deliver()`, + * `forEach(deliver)`, a shorthand `{ deliver }`, and so on. Skips positions + * that name something rather than reference a value: a declaration's own + * `id`/params, the non-computed `.property` of a member expression (so + * `someService.deliver()` never counts as a reference to an unrelated local + * `deliver`), and the target of an assignment, which writes to a name + * instead of reading it. + */ +function collectReferencedNames(node: unknown, out: Set): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectReferencedNames(item, out); + return; + } + if (!isNode(node)) return; + const n = node; + + if (n.type === "Identifier") { + out.add(n.name); + return; + } + if (n.type === "MemberExpression" && !n.computed) { + collectReferencedNames(n.object, out); + return; + } + if (n.type === "Property" && !n.computed) { + // `{ deliver: fn }` -- the key is a name, not a reference; only the + // value is (for shorthand `{ deliver }`, the value is the same name, + // so this still counts it). + collectReferencedNames(n.value, out); + return; + } + if (n.type === "VariableDeclarator") { + if ((n as VariableDeclarator).init != null) { + collectReferencedNames((n as VariableDeclarator).init, out); + } + return; + } + if (n.type === "ClassDeclaration" || n.type === "ClassExpression") { + // The class's own name is a declaration, not a mention of it. + collectReferencedNames(n.superClass, out); + collectReferencedNames(n.body, out); + return; + } + if ( + (n.type === "MethodDefinition" || n.type === "PropertyDefinition") && + !n.computed + ) { + // Same as an object literal's `Property`: the key names a member, and + // only what it holds can reference something. + collectReferencedNames(n.value, out); + return; + } + if (n.type === "AssignmentExpression" && n.operator === "=") { + // `x = fn` and `obj.x = fn` write to a name rather than mention it. + if (getAssignmentTargetName(n.left as Node) != null) { + collectReferencedNames(n.right, out); + return; + } + } + if (isFunctionLikeNode(n)) { + // Stop at a nested function's own boundary: whether a name it + // references counts is decided separately, only once that function + // itself is found to be reachable. + return; + } + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectReferencedNames(record[key], out); + } +} + +/** + * The name an assignment writes to: `x` for `x = ...`, and the root object + * for `obj.a.b = ...`. `null` for anything more exotic. + */ +function getAssignmentTargetName(target: Node): string | null { + let current: Node = target; + while (current.type === "MemberExpression") current = current.object as Node; + return current.type === "Identifier" ? current.name : null; +} + +/** Every identifier a declaration pattern binds (`a`, `{ a, b: c }`, `[a]`). */ +function collectBoundNames(pattern: unknown, out: string[]): void { + if (pattern == null || typeof pattern !== "object" || !isNode(pattern)) { + return; + } + const p = pattern as Node; + switch (p.type) { + case "Identifier": + out.push(p.name); + return; + case "AssignmentPattern": + collectBoundNames(p.left, out); + return; + case "RestElement": + collectBoundNames(p.argument, out); + return; + case "ArrayPattern": + for (const element of p.elements) collectBoundNames(element, out); + return; + case "ObjectPattern": + for (const prop of p.properties) { + collectBoundNames( + (prop as { value?: unknown; argument?: unknown }).value ?? + (prop as { argument?: unknown }).argument, + out, + ); + } + return; + } +} + +/** + * Finds the function literals a value holds itself: the value is the + * function, or it sits in an object or array literal, a conditional, or a + * class body. Stops at a call, so a function handed to one as an argument, + * or invoked by it, is not held by whatever the call's result is bound to. + * Those count on their own wherever they appear. + */ +function collectHeldFunctions(node: unknown, out: FunctionLikeNode[]): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectHeldFunctions(item, out); + return; + } + if (!isNode(node)) return; + const n = node; + + if (isFunctionLikeNode(n)) { + out.push(n); + return; + } + if (n.type === "CallExpression" || n.type === "NewExpression") return; + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectHeldFunctions(record[key], out); + } +} + +/** + * Collects the functions each name in `node`'s own scope holds: `function + * name() {}`, `class Name {}`, and the value of `const name = ...` or a + * later `name = ...` or `name.prop = ...`, including a function inside an + * object or array literal (see `collectHeldFunctions`). A function held + * under a name counts as used as soon as the name is mentioned, however it + * is mentioned, so this never has to work out how the name reaches the + * function. Does not descend into a found function's own body: a name bound + * inside it is only found once that function is itself resolved as + * reachable, so it can be layered on top of (and correctly shadow) the outer + * scope's names. + */ +function collectFunctionsByName( + node: unknown, + out: Map, +): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectFunctionsByName(item, out); + return; + } + if (!isNode(node)) return; + const n = node; + + const bindTo = (names: string[], from: unknown): void => { + const functions: FunctionLikeNode[] = []; + collectHeldFunctions(from, functions); + if (functions.length < 1) return; + for (const name of names) { + out.set(name, [...(out.get(name) ?? []), ...functions]); + } + }; + + if (n.type === "FunctionDeclaration") { + if (n.id?.name != null) bindTo([n.id.name], n); + return; + } + if (n.type === "ClassDeclaration") { + if (n.id?.name != null) bindTo([n.id.name], n); + return; + } + if (isFunctionLikeNode(n)) return; + if (n.type === "VariableDeclarator") { + const decl = n as VariableDeclarator; + if (decl.init != null) { + const names: string[] = []; + collectBoundNames(decl.id, names); + bindTo(names, decl.init); + collectFunctionsByName(decl.init, out); + } + return; + } + if (n.type === "AssignmentExpression") { + const name = getAssignmentTargetName(n.left as Node); + if (name != null) bindTo([name], n.right); + collectFunctionsByName(n.right, out); + return; + } + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectFunctionsByName(record[key], out); + } +} + +/** + * Finds function literals directly nested in a reachable statement, without + * descending past them -- their own reachability is decided separately. + */ +export function collectNestedFunctions( + node: unknown, + out: FunctionLikeNode[], +): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectNestedFunctions(item, out); + return; + } + if (!isNode(node)) return; + const n = node; + + if (isFunctionLikeNode(n)) { + out.push(n); + return; + } + + const record = n as unknown as Record; + for (const key in record) { + if (key === "parent") continue; + collectNestedFunctions(record[key], out); + } +} + +/** + * Computes the full set of function nodes that are actually reachable from + * `root`: `root` itself feeds a worklist, and each function it (or a + * function already on the worklist) uses from *its own* reachable + * statements -- never from a dead branch or some other not-yet-reached + * function's body -- gets queued in turn. `outerFunctionsByName` is layered + * fresh for each scope, so a name bound at an inner scope shadows a + * same-named one further out instead of overwriting it globally, and a + * dead branch that merely mentions a name never queues what it holds. + */ +export function computeUsedFunctions(root: Node): Set { + const used = new Set(); + const visited = new Set(); + + const processScope = ( + scopeRoot: Node, + outerFunctionsByName: ReadonlyMap, + ): void => { + if (visited.has(scopeRoot)) return; + visited.add(scopeRoot); + + const statements: Node[] = []; + collectReachableStatements(scopeRoot, statements); + + const functionsHere = new Map(); + for (const statement of statements) { + collectFunctionsByName(statement, functionsHere); + } + const functionsByName = new Map(outerFunctionsByName); + for (const [name, functions] of functionsHere) { + functionsByName.set(name, functions); + } + + const referencedNames = new Set(); + for (const statement of statements) { + collectReferencedNames(statement, referencedNames); + } + // A function held under a name counts as reached as soon as that name + // is mentioned at all -- called, passed along, aliased, destructured, + // passed to `console.log`, stored in a variable, anything -- not only + // when it's actually invoked. That's what lets `recipients.map(deliver)` + // and `const { deliver } = handlers` resolve as used without this code + // having to know that `map` invokes its argument or how a destructured + // property gets from the object to the call. Telling a real invocation + // apart from merely holding a reference would need following every + // shape a function value can travel in and knowing which APIs call what + // they're given, which is more than this rule should carry, and any + // shape it missed would report code that delivers. The cost is a + // narrow false negative: a function that's only logged or reassigned, + // never called, is not reported. That is accepted deliberately, since + // missing a case here is the safe direction. Leave this as is. + const reached = new Set(); + for (const [name, functions] of functionsByName) { + if (!referencedNames.has(name)) continue; + for (const fn of functions) reached.add(fn); + } + + // Every other function literal counts wherever it appears: a callback + // handed to `map`, `forEach`, `queue.push` or a call the rule has never + // heard of, an immediately invoked function, a returned closure. The + // rule cannot show that the receiving code never runs it, and it does + // not check whether the result is awaited: a delivery call that is + // never awaited is left alone too. Leave this as is. + const held = new Set(); + for (const functions of functionsHere.values()) { + for (const fn of functions) held.add(fn); + } + for (const statement of statements) { + const nested: FunctionLikeNode[] = []; + collectNestedFunctions(statement, nested); + for (const fn of nested) { + if (!held.has(fn)) reached.add(fn); + } + } + + for (const fn of reached) { + used.add(fn); + processScope(fn.body as Node, functionsByName); + } + }; + + processScope(root, new Map()); + return used; +} + +/** + * A node's `[start, end)` character offsets into the whole source file. + * Both engines always populate this -- ESLint forces it on regardless of + * parser options, and Deno.lint exposes it the same way as every other + * child property (see the `for...in` note on why plain property access + * still works even though it's not an own enumerable property). + */ +export function getRange(node: Node): readonly [number, number] { + return (node as unknown as { range: [number, number] }).range; +} diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index 91d8f4459..9e44279e0 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -1,84 +1,22 @@ import type { Rule } from "eslint"; import { - hasIdentifierProperty, - hasMemberExpressionCallee, - hasMethodName, - isFunction, - isNode, -} from "../lib/pred.ts"; -import { trackFederationVariables } from "../lib/tracker.ts"; -import type { - AssignmentPattern, - CallExpression, - Expression, - FunctionNode, - Identifier, - Node, - VariableDeclarator, -} from "../lib/types.ts"; + createOutboxListenerVisitor, + DELIVERY_METHOD_NAMES, + unwrapContextParam, +} from "../lib/outbox-listener.ts"; +import { isNode } from "../lib/pred.ts"; +import { + collectNestedFunctions, + collectReachableStatements, + computeUsedFunctions, + type FunctionLikeNode, + getRange, +} from "../lib/reachability.ts"; +import type { Node } from "../lib/types.ts"; const MESSAGE = "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity()."; -const isChainedFromOutboxListeners = ( - expr: Expression, - federationTracker: ReturnType, -): boolean => { - if (expr.type !== "CallExpression") return false; - if (!hasMemberExpressionCallee(expr) || !hasIdentifierProperty(expr)) { - return false; - } - const methodName = expr.callee.property.name; - if (methodName === "setOutboxListeners") { - return federationTracker.isFederationObject(expr.callee.object); - } - if ( - methodName === "authorize" || methodName === "onError" || - methodName === "on" - ) { - return isChainedFromOutboxListeners(expr.callee.object, federationTracker); - } - return false; -}; - -const DELIVERY_METHOD_NAMES = new Set(["sendActivity", "forwardActivity"]); - -type FunctionLikeNode = - | FunctionNode - | (Node & { - type: "FunctionDeclaration"; - id: Identifier | null; - params: unknown[]; - body: unknown; - }); - -const FUNCTION_NODE_TYPES = new Set([ - "FunctionDeclaration", - "FunctionExpression", - "ArrowFunctionExpression", -]); - -const isFunctionLikeNode = (node: Node): node is FunctionLikeNode => - FUNCTION_NODE_TYPES.has(node.type); - -const getMemberPropertyName = (expr: Expression): string | null => { - if (expr.type !== "MemberExpression") return null; - const property = expr.property as Node; - if (property.type === "Identifier") return property.name; - if (property.type === "Literal" && typeof property.value === "string") { - return property.value; - } - return null; -}; - -function unwrapContextParam(node: Node | undefined): Node | null { - let current: Node | null = node ?? null; - while (current?.type === "AssignmentPattern") { - current = (current as AssignmentPattern).left as Node; - } - return current; -} - function escapeRegExp(value: string): string { return value.replaceAll(/[.*+?^${}()|[\]\\]/g, "\\$&"); } @@ -228,569 +166,6 @@ function buildContextExpressionPattern(contextName: string): string { .raw`(?:${boundedName}|\(\s*${boundedName}(?:\s+as\s+[^)]+)?\s*\))`; } -/** - * Resolves an expression to the function it refers to: a direct function - * literal, a local variable bound to one, or a property of a local object - * literal bound to one (e.g. `handlers.deliver` where - * `const handlers = { deliver() {} }`). Used to resolve a listener argument - * (`.on(Activity, handler)`). - */ -const resolveFunctionBinding = ( - expr: Expression, - bindings: Map, - seen = new Set(), -): FunctionLikeNode | null => { - if (isFunction(expr)) return expr; - if (expr.type === "Identifier") { - if (seen.has(expr.name)) return null; - seen.add(expr.name); - const binding = bindings.get(expr.name); - if (binding == null || !isNode(binding)) return null; - if ( - isFunction(binding as Expression) || - (binding as { type?: string }).type === "FunctionDeclaration" - ) { - return binding as FunctionLikeNode; - } - if (binding.type === "Identifier") { - return resolveFunctionBinding(binding, bindings, seen); - } - return null; - } - if ( - expr.type === "MemberExpression" && expr.object.type === "Identifier" && - !expr.computed - ) { - const binding = bindings.get(expr.object.name); - if ( - binding == null || !isNode(binding) || binding.type !== "ObjectExpression" - ) { - return null; - } - const propertyName = getMemberPropertyName(expr); - if (propertyName == null) return null; - for (const prop of binding.properties) { - if (!isNode(prop) || prop.type !== "Property") continue; - const keyName = prop.key.type === "Identifier" - ? prop.key.name - : prop.key.type === "Literal" && typeof prop.key.value === "string" - ? prop.key.value - : null; - if (keyName !== propertyName || !isNode(prop.value)) continue; - const value = prop.value as unknown; - if ( - isFunction(value as Expression) || - (value as { type?: string }).type === "FunctionDeclaration" - ) { - return value as FunctionLikeNode; - } - } - } - return null; -}; - -// --------------------------------------------------------------------------- -// Reachability: which statements can actually run, following control flow -// (if/else, try/catch/finally, switch, loops) but never descending into a -// nested function's own body, and pruning dead code (a statically-falsy `if` -// branch, or anything after a statement that always returns/throws). -// -// A control-flow statement's head expressions (an `if` test, a `switch` -// discriminant and case tests, a loop's `init`/`test`/`update`/`right`) run -// whenever the statement itself does, whichever branch is taken, so they are -// collected alongside the bodies. Collecting one never revives the branch -// behind it: `if (false)` still hides its consequent. -// --------------------------------------------------------------------------- - -const isStaticallyFalsy = (test: Expression): boolean => - test.type === "Literal" && !test.value; - -const isStaticallyTruthy = (test: Expression): boolean => - test.type === "Literal" && Boolean(test.value); - -/** - * Whether every path through this statement unconditionally returns or - * throws, meaning anything textually after it in the same statement list - * never runs. Deliberately conservative: when it can't prove that, it - * answers `false`, which keeps the following code counted as reachable - * (a missed dead-code case is safer than wrongly hiding live code). - */ -function alwaysExits(node: Node): boolean { - switch (node.type) { - case "ReturnStatement": - case "ThrowStatement": - case "BreakStatement": - case "ContinueStatement": - return true; - - case "BlockStatement": - return node.body.some((statement) => alwaysExits(statement as Node)); - - case "IfStatement": { - const test = node.test as Expression; - if (isStaticallyFalsy(test)) { - return node.alternate != null && alwaysExits(node.alternate as Node); - } - if (isStaticallyTruthy(test)) { - return alwaysExits(node.consequent as Node); - } - if (node.alternate == null) return false; - return alwaysExits(node.consequent as Node) && - alwaysExits(node.alternate as Node); - } - - case "TryStatement": - // A `finally` that always exits dominates the whole statement. Beyond - // that, a `try` block can throw partway through and jump to `catch`, - // so proving more than this would need tracking which statements can - // throw -- stay conservative and say "not sure" instead. - return node.finalizer != null && alwaysExits(node.finalizer as Node); - - default: - return false; - } -} - -function collectReachableStatements(node: Node, out: Node[]): void { - switch (node.type) { - case "BlockStatement": - for (const [index, statement] of node.body.entries()) { - collectReachableStatements(statement as Node, out); - if (alwaysExits(statement as Node)) { - // Function declarations hoist: one written below an exit is still - // callable from the code above it. - for (const rest of node.body.slice(index + 1)) { - if ((rest as Node).type === "FunctionDeclaration") { - out.push(rest as Node); - } - } - return; - } - } - return; - - case "IfStatement": { - const test = node.test as Expression; - out.push(test); - if (!isStaticallyFalsy(test)) { - collectReachableStatements(node.consequent as Node, out); - } - if (node.alternate != null && !isStaticallyTruthy(test)) { - collectReachableStatements(node.alternate as Node, out); - } - return; - } - - case "TryStatement": - collectReachableStatements(node.block as Node, out); - if (node.handler != null) { - collectReachableStatements(node.handler.body as Node, out); - } - if (node.finalizer != null) { - collectReachableStatements(node.finalizer as Node, out); - } - return; - - case "SwitchStatement": - out.push(node.discriminant as Node); - for (const switchCase of node.cases) { - if (switchCase.test != null) out.push(switchCase.test as Node); - for (const statement of switchCase.consequent) { - collectReachableStatements(statement as Node, out); - if (alwaysExits(statement as Node)) break; - } - } - return; - - case "WhileStatement": - case "DoWhileStatement": - out.push(node.test as Node); - collectReachableStatements(node.body as Node, out); - return; - - case "ForStatement": - for (const head of [node.init, node.test, node.update]) { - if (head != null) out.push(head as Node); - } - collectReachableStatements(node.body as Node, out); - return; - - case "ForInStatement": - case "ForOfStatement": - // Only `right` is evaluated as a value; `left` declares or assigns the - // loop variable. - out.push(node.right as Node); - collectReachableStatements(node.body as Node, out); - return; - - case "LabeledStatement": - collectReachableStatements(node.body as Node, out); - return; - - case "WithStatement": - out.push(node.object as Node); - collectReachableStatements(node.body as Node, out); - return; - - default: - out.push(node); - return; - } -} - -// --------------------------------------------------------------------------- -// What a listener (or a helper's own body) resolves to when scanned for a -// delivery call: two rules decide which nested function bodies are folded -// into the scan instead of being masked out. -// -// The rule reports only when neither can account for a delivery call, so -// both err toward treating a function as used. Working out how a function -// value travels through arbitrary JavaScript (an alias, a destructured -// property, an array, a wrapper call) is open-ended, and so is working out -// what a call does with a callback it receives. Showing that a name never -// appears anywhere that runs is not. So a function held under a name is -// used as soon as that name is mentioned, without tracing how it is then -// passed around, and any other function literal, such as a callback handed -// to a call, is used wherever it appears, since the rule cannot show that -// the receiving call never runs it. A missed warning is the safe direction; -// a warning on code that delivers is not. -// --------------------------------------------------------------------------- - -/** - * Collects plain-value references to identifiers: `deliver()`, - * `forEach(deliver)`, a shorthand `{ deliver }`, and so on. Skips positions - * that name something rather than reference a value: a declaration's own - * `id`/params, the non-computed `.property` of a member expression (so - * `someService.deliver()` never counts as a reference to an unrelated local - * `deliver`), and the target of an assignment, which writes to a name - * instead of reading it. - */ -function collectReferencedNames(node: unknown, out: Set): void { - if (node == null || typeof node !== "object") return; - if (Array.isArray(node)) { - for (const item of node) collectReferencedNames(item, out); - return; - } - if (!isNode(node)) return; - const n = node; - - if (n.type === "Identifier") { - out.add(n.name); - return; - } - if (n.type === "MemberExpression" && !n.computed) { - collectReferencedNames(n.object, out); - return; - } - if (n.type === "Property" && !n.computed) { - // `{ deliver: fn }` -- the key is a name, not a reference; only the - // value is (for shorthand `{ deliver }`, the value is the same name, - // so this still counts it). - collectReferencedNames(n.value, out); - return; - } - if (n.type === "VariableDeclarator") { - if ((n as VariableDeclarator).init != null) { - collectReferencedNames((n as VariableDeclarator).init, out); - } - return; - } - if (n.type === "ClassDeclaration" || n.type === "ClassExpression") { - // The class's own name is a declaration, not a mention of it. - collectReferencedNames(n.superClass, out); - collectReferencedNames(n.body, out); - return; - } - if ( - (n.type === "MethodDefinition" || n.type === "PropertyDefinition") && - !n.computed - ) { - // Same as an object literal's `Property`: the key names a member, and - // only what it holds can reference something. - collectReferencedNames(n.value, out); - return; - } - if (n.type === "AssignmentExpression" && n.operator === "=") { - // `x = fn` and `obj.x = fn` write to a name rather than mention it. - if (getAssignmentTargetName(n.left as Node) != null) { - collectReferencedNames(n.right, out); - return; - } - } - if (isFunctionLikeNode(n)) { - // Stop at a nested function's own boundary: whether a name it - // references counts is decided separately, only once that function - // itself is found to be reachable. - return; - } - - const record = n as unknown as Record; - for (const key in record) { - if (key === "parent") continue; - collectReferencedNames(record[key], out); - } -} - -/** - * The name an assignment writes to: `x` for `x = ...`, and the root object - * for `obj.a.b = ...`. `null` for anything more exotic. - */ -function getAssignmentTargetName(target: Node): string | null { - let current: Node = target; - while (current.type === "MemberExpression") current = current.object as Node; - return current.type === "Identifier" ? current.name : null; -} - -/** Every identifier a declaration pattern binds (`a`, `{ a, b: c }`, `[a]`). */ -function collectBoundNames(pattern: unknown, out: string[]): void { - if (pattern == null || typeof pattern !== "object" || !isNode(pattern)) { - return; - } - const p = pattern as Node; - switch (p.type) { - case "Identifier": - out.push(p.name); - return; - case "AssignmentPattern": - collectBoundNames(p.left, out); - return; - case "RestElement": - collectBoundNames(p.argument, out); - return; - case "ArrayPattern": - for (const element of p.elements) collectBoundNames(element, out); - return; - case "ObjectPattern": - for (const prop of p.properties) { - collectBoundNames( - (prop as { value?: unknown; argument?: unknown }).value ?? - (prop as { argument?: unknown }).argument, - out, - ); - } - return; - } -} - -/** - * Finds the function literals a value holds itself: the value is the - * function, or it sits in an object or array literal, a conditional, or a - * class body. Stops at a call, so a function handed to one as an argument, - * or invoked by it, is not held by whatever the call's result is bound to. - * Those count on their own wherever they appear. - */ -function collectHeldFunctions(node: unknown, out: FunctionLikeNode[]): void { - if (node == null || typeof node !== "object") return; - if (Array.isArray(node)) { - for (const item of node) collectHeldFunctions(item, out); - return; - } - if (!isNode(node)) return; - const n = node; - - if (isFunctionLikeNode(n)) { - out.push(n); - return; - } - if (n.type === "CallExpression" || n.type === "NewExpression") return; - - const record = n as unknown as Record; - for (const key in record) { - if (key === "parent") continue; - collectHeldFunctions(record[key], out); - } -} - -/** - * Collects the functions each name in `node`'s own scope holds: `function - * name() {}`, `class Name {}`, and the value of `const name = ...` or a - * later `name = ...` or `name.prop = ...`, including a function inside an - * object or array literal (see `collectHeldFunctions`). A function held - * under a name counts as used as soon as the name is mentioned, however it - * is mentioned, so this never has to work out how the name reaches the - * function. Does not descend into a found function's own body: a name bound - * inside it is only found once that function is itself resolved as - * reachable, so it can be layered on top of (and correctly shadow) the outer - * scope's names. - */ -function collectFunctionsByName( - node: unknown, - out: Map, -): void { - if (node == null || typeof node !== "object") return; - if (Array.isArray(node)) { - for (const item of node) collectFunctionsByName(item, out); - return; - } - if (!isNode(node)) return; - const n = node; - - const bindTo = (names: string[], from: unknown): void => { - const functions: FunctionLikeNode[] = []; - collectHeldFunctions(from, functions); - if (functions.length < 1) return; - for (const name of names) { - out.set(name, [...(out.get(name) ?? []), ...functions]); - } - }; - - if (n.type === "FunctionDeclaration") { - if (n.id?.name != null) bindTo([n.id.name], n); - return; - } - if (n.type === "ClassDeclaration") { - if (n.id?.name != null) bindTo([n.id.name], n); - return; - } - if (isFunctionLikeNode(n)) return; - if (n.type === "VariableDeclarator") { - const decl = n as VariableDeclarator; - if (decl.init != null) { - const names: string[] = []; - collectBoundNames(decl.id, names); - bindTo(names, decl.init); - collectFunctionsByName(decl.init, out); - } - return; - } - if (n.type === "AssignmentExpression") { - const name = getAssignmentTargetName(n.left as Node); - if (name != null) bindTo([name], n.right); - collectFunctionsByName(n.right, out); - return; - } - - const record = n as unknown as Record; - for (const key in record) { - if (key === "parent") continue; - collectFunctionsByName(record[key], out); - } -} - -/** - * Finds function literals directly nested in a reachable statement, without - * descending past them -- their own reachability is decided separately. - */ -function collectNestedFunctions( - node: unknown, - out: FunctionLikeNode[], -): void { - if (node == null || typeof node !== "object") return; - if (Array.isArray(node)) { - for (const item of node) collectNestedFunctions(item, out); - return; - } - if (!isNode(node)) return; - const n = node; - - if (isFunctionLikeNode(n)) { - out.push(n); - return; - } - - const record = n as unknown as Record; - for (const key in record) { - if (key === "parent") continue; - collectNestedFunctions(record[key], out); - } -} - -/** - * Computes the full set of function nodes that are actually reachable from - * `root`: `root` itself feeds a worklist, and each function it (or a - * function already on the worklist) uses from *its own* reachable - * statements -- never from a dead branch or some other not-yet-reached - * function's body -- gets queued in turn. `outerFunctionsByName` is layered - * fresh for each scope, so a name bound at an inner scope shadows a - * same-named one further out instead of overwriting it globally, and a - * dead branch that merely mentions a name never queues what it holds. - */ -function computeUsedFunctions(root: Node): Set { - const used = new Set(); - const visited = new Set(); - - const processScope = ( - scopeRoot: Node, - outerFunctionsByName: ReadonlyMap, - ): void => { - if (visited.has(scopeRoot)) return; - visited.add(scopeRoot); - - const statements: Node[] = []; - collectReachableStatements(scopeRoot, statements); - - const functionsHere = new Map(); - for (const statement of statements) { - collectFunctionsByName(statement, functionsHere); - } - const functionsByName = new Map(outerFunctionsByName); - for (const [name, functions] of functionsHere) { - functionsByName.set(name, functions); - } - - const referencedNames = new Set(); - for (const statement of statements) { - collectReferencedNames(statement, referencedNames); - } - // A function held under a name counts as reached as soon as that name - // is mentioned at all -- called, passed along, aliased, destructured, - // passed to `console.log`, stored in a variable, anything -- not only - // when it's actually invoked. That's what lets `recipients.map(deliver)` - // and `const { deliver } = handlers` resolve as used without this code - // having to know that `map` invokes its argument or how a destructured - // property gets from the object to the call. Telling a real invocation - // apart from merely holding a reference would need following every - // shape a function value can travel in and knowing which APIs call what - // they're given, which is more than this rule should carry, and any - // shape it missed would report code that delivers. The cost is a - // narrow false negative: a function that's only logged or reassigned, - // never called, is not reported. That is accepted deliberately, since - // missing a case here is the safe direction. Leave this as is. - const reached = new Set(); - for (const [name, functions] of functionsByName) { - if (!referencedNames.has(name)) continue; - for (const fn of functions) reached.add(fn); - } - - // Every other function literal counts wherever it appears: a callback - // handed to `map`, `forEach`, `queue.push` or a call the rule has never - // heard of, an immediately invoked function, a returned closure. The - // rule cannot show that the receiving code never runs it, and it does - // not check whether the result is awaited: a delivery call that is - // never awaited is left alone too. Leave this as is. - const held = new Set(); - for (const functions of functionsHere.values()) { - for (const fn of functions) held.add(fn); - } - for (const statement of statements) { - const nested: FunctionLikeNode[] = []; - collectNestedFunctions(statement, nested); - for (const fn of nested) { - if (!held.has(fn)) reached.add(fn); - } - } - - for (const fn of reached) { - used.add(fn); - processScope(fn.body as Node, functionsByName); - } - }; - - processScope(root, new Map()); - return used; -} - -/** - * A node's `[start, end)` character offsets into the whole source file. - * Both engines always populate this -- ESLint forces it on regardless of - * parser options, and Deno.lint exposes it the same way as every other - * child property (see the `for...in` note on why plain property access - * still works even though it's not an own enumerable property). - */ -function getRange(node: Node): readonly [number, number] { - return (node as unknown as { range: [number, number] }).range; -} - /** * Builds the source text to scan for a delivery call: the reachable * statements of `root`, with every nested function literal either folded @@ -932,72 +307,18 @@ function createRule( }, ) { return (context: Context) => { - const federationTracker = trackFederationVariables(); - const bindings = new Map(); - const pendingCalls: CallExpression[] = []; const sourceCode = (context as { sourceCode: { getText(node: unknown): string } }) .sourceCode; - const inspectCall = (node: CallExpression): void => { - if ( - !hasMemberExpressionCallee(node) || - !hasIdentifierProperty(node) || - !hasMethodName("on")(node) || - node.arguments.length < 2 - ) { - return; - } - if ( - !isChainedFromOutboxListeners(node.callee.object, federationTracker) - ) { - return; - } - - const listener = node.arguments[1] as unknown; - const resolvedListener = - isNode(listener) && isFunction(listener as Expression) - ? listener as FunctionLikeNode - : isNode(listener) - ? resolveFunctionBinding(listener as Expression, bindings) - : null; - if (resolvedListener == null) return; - - if (listenerCallsDeliveryMethod(sourceCode, resolvedListener)) { - return; - } + return createOutboxListenerVisitor((listener) => { + if (listenerCallsDeliveryMethod(sourceCode, listener)) return; (context as { report: (arg: unknown) => void }).report({ - node: resolvedListener, + node: listener, ...buildReport, }); - }; - - return { - VariableDeclarator(node: VariableDeclarator): void { - federationTracker.VariableDeclarator(node); - if (node.id.type === "Identifier" && node.init != null) { - bindings.set(node.id.name, node.init); - } - }, - - FunctionDeclaration( - node: Node & { - type: "FunctionDeclaration"; - id: Identifier | null; - }, - ): void { - if (node.id != null) bindings.set(node.id.name, node); - }, - - CallExpression(node: CallExpression): void { - pendingCalls.push(node); - }, - - "Program:exit"(): void { - for (const node of pendingCalls) inspectCall(node); - }, - }; + }); }; } From f70a1f0ea7a899de63556ddd85db1dc62ec51b09 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sat, 26 Sep 2026 09:46:03 +0900 Subject: [PATCH 02/11] Keep a spliced-in method body apart from its key The delivery scan splices the text of each used function into the statement that declares it, replacing the function's range. A method's range starts right after its key on some parsers, so a body that began with the delivery call itself fused with the key: `deliver() { ctx.sendActivity(...); }` scanned as `deliverctx.sendActivity(...)`, where the context name no longer sits on a word boundary. Under ESLint that reported a listener that plainly delivers, for a class method, a static method, an object method, an async one and a getter alike. Deno was unaffected, and so was a body that began with `return` or `await`, by luck of the added space. Surround the spliced text with line breaks, and cover the five method shapes with tests that fail on Node without the change. Assisted-by: Claude Code:claude-sonnet-5 --- .../outbox-listener-delivery-required.ts | 5 +- .../outbox-listener-delivery-required.test.ts | 121 ++++++++++++++++++ 2 files changed, 125 insertions(+), 1 deletion(-) diff --git a/packages/lint/src/rules/outbox-listener-delivery-required.ts b/packages/lint/src/rules/outbox-listener-delivery-required.ts index 9e44279e0..4d61e3312 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-required.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-required.ts @@ -213,8 +213,11 @@ function collectDeliveryScanCode( const replacement = used.has(fn) ? collectDeliveryScanCode(sourceCode, fn.body as Node, used, visited) : ""; + // A method's range starts right after its key (`go` in `go() {}`) + // on some parsers, so keep the spliced text apart from what + // surrounds it, or `go` and `ctx` fuse into a single `goctx`. result = result.slice(0, fnStart - statementStart) + - (replacement.length > 0 ? replacement : "()=>{}") + + `\n${replacement.length > 0 ? replacement : "()=>{}"}\n` + result.slice(fnEnd - statementStart); } return result; diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 2346eb150..849479010 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -2086,3 +2086,124 @@ federation "Outbox listeners should deliver posted activities explicitly with ctx.sendActivity() or ctx.forwardActivity().", }), ); + +test( + `${ruleName}: ✅ Good - bare delivery call in a class method`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + class Sender { + deliver() { + ctx.sendActivity(sender, inbox, activity); + } + } + new Sender().deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - bare delivery call in a static class method`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + class Sender { + static deliver() { + ctx.sendActivity(sender, inbox, activity); + } + } + Sender.deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - bare delivery call in an object method`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { + deliver() { + ctx.sendActivity(sender, inbox, activity); + }, + }; + handlers.deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - bare delivery call in an async object method`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { + async deliver() { + ctx.sendActivity(sender, inbox, activity); + }, + }; + await handlers.deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - bare delivery call in an object getter`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { + get deliver() { + ctx.sendActivity(sender, inbox, activity); + return 1; + }, + }; + console.log(handlers.deliver); + }); +`, + rule, + ruleName, + }), +); From 186bab48dbcb5db710de309fefd7c9a7e2e3d043 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sat, 26 Sep 2026 09:46:34 +0900 Subject: [PATCH 03/11] Let rules visit each scope of the used-function walk A rule that checks how a listener's delivery calls end up needs more than the set of used functions: for each scope it needs the statements that can run and the functions each name holds there. Turn the walk behind computeUsedFunctions() into walkUsedScopes(), which calls a visitor for every scope, and keep computeUsedFunctions() as a wrapper so the existing rule is unchanged. Let collectReferencedNames() optionally cross function boundaries, for callers that need every mention of a name in a listener, and export the helpers a second rule needs. https://github.com/fedify-dev/fedify/issues/1057 Assisted-by: Claude Code:claude-sonnet-5 --- packages/lint/src/lib/reachability.ts | 97 +++++++++++++++++++-------- 1 file changed, 70 insertions(+), 27 deletions(-) diff --git a/packages/lint/src/lib/reachability.ts b/packages/lint/src/lib/reachability.ts index 32d527db3..4be84cbdb 100644 --- a/packages/lint/src/lib/reachability.ts +++ b/packages/lint/src/lib/reachability.ts @@ -22,7 +22,7 @@ const FUNCTION_NODE_TYPES = new Set([ "ArrowFunctionExpression", ]); -const isFunctionLikeNode = (node: Node): node is FunctionLikeNode => +export const isFunctionLikeNode = (node: Node): node is FunctionLikeNode => FUNCTION_NODE_TYPES.has(node.type); // --------------------------------------------------------------------------- @@ -201,10 +201,14 @@ export function collectReachableStatements(node: Node, out: Node[]): void { * `deliver`), and the target of an assignment, which writes to a name * instead of reading it. */ -function collectReferencedNames(node: unknown, out: Set): void { +export function collectReferencedNames( + node: unknown, + out: Set, + crossFunctions = false, +): void { if (node == null || typeof node !== "object") return; if (Array.isArray(node)) { - for (const item of node) collectReferencedNames(item, out); + for (const item of node) collectReferencedNames(item, out, crossFunctions); return; } if (!isNode(node)) return; @@ -215,26 +219,30 @@ function collectReferencedNames(node: unknown, out: Set): void { return; } if (n.type === "MemberExpression" && !n.computed) { - collectReferencedNames(n.object, out); + collectReferencedNames(n.object, out, crossFunctions); return; } if (n.type === "Property" && !n.computed) { // `{ deliver: fn }` -- the key is a name, not a reference; only the // value is (for shorthand `{ deliver }`, the value is the same name, // so this still counts it). - collectReferencedNames(n.value, out); + collectReferencedNames(n.value, out, crossFunctions); return; } if (n.type === "VariableDeclarator") { if ((n as VariableDeclarator).init != null) { - collectReferencedNames((n as VariableDeclarator).init, out); + collectReferencedNames( + (n as VariableDeclarator).init, + out, + crossFunctions, + ); } return; } if (n.type === "ClassDeclaration" || n.type === "ClassExpression") { // The class's own name is a declaration, not a mention of it. - collectReferencedNames(n.superClass, out); - collectReferencedNames(n.body, out); + collectReferencedNames(n.superClass, out, crossFunctions); + collectReferencedNames(n.body, out, crossFunctions); return; } if ( @@ -243,27 +251,32 @@ function collectReferencedNames(node: unknown, out: Set): void { ) { // Same as an object literal's `Property`: the key names a member, and // only what it holds can reference something. - collectReferencedNames(n.value, out); + collectReferencedNames(n.value, out, crossFunctions); return; } if (n.type === "AssignmentExpression" && n.operator === "=") { // `x = fn` and `obj.x = fn` write to a name rather than mention it. if (getAssignmentTargetName(n.left as Node) != null) { - collectReferencedNames(n.right, out); + collectReferencedNames(n.right, out, crossFunctions); return; } } if (isFunctionLikeNode(n)) { - // Stop at a nested function's own boundary: whether a name it + // Stop at a nested function's own boundary by default: whether a name it // references counts is decided separately, only once that function - // itself is found to be reachable. + // itself is found to be reachable. A caller that wants every mention in + // a function, such as whether a stored value is used anywhere, asks to + // cross it. + if (crossFunctions) { + collectReferencedNames(n.body, out, crossFunctions); + } return; } const record = n as unknown as Record; for (const key in record) { if (key === "parent") continue; - collectReferencedNames(record[key], out); + collectReferencedNames(record[key], out, crossFunctions); } } @@ -271,7 +284,7 @@ function collectReferencedNames(node: unknown, out: Set): void { * The name an assignment writes to: `x` for `x = ...`, and the root object * for `obj.a.b = ...`. `null` for anything more exotic. */ -function getAssignmentTargetName(target: Node): string | null { +export function getAssignmentTargetName(target: Node): string | null { let current: Node = target; while (current.type === "MemberExpression") current = current.object as Node; return current.type === "Identifier" ? current.name : null; @@ -432,21 +445,42 @@ export function collectNestedFunctions( } /** - * Computes the full set of function nodes that are actually reachable from - * `root`: `root` itself feeds a worklist, and each function it (or a - * function already on the worklist) uses from *its own* reachable - * statements -- never from a dead branch or some other not-yet-reached - * function's body -- gets queued in turn. `outerFunctionsByName` is layered - * fresh for each scope, so a name bound at an inner scope shadows a - * same-named one further out instead of overwriting it globally, and a - * dead branch that merely mentions a name never queues what it holds. + * One scope of the used-function walk: the body of the root, or of a function + * found to be used. */ -export function computeUsedFunctions(root: Node): Set { +export type UsedScope = { + /** The function whose body this scope is, or `null` for the root. */ + fn: FunctionLikeNode | null; + /** The statements and control-flow head expressions that can run here. */ + statements: readonly Node[]; + /** + * The functions each name holds at this point, with a name bound in an + * inner scope shadowing a same-named one further out. + */ + functionsByName: ReadonlyMap; +}; + +/** + * Walks every scope that is actually reachable from `root`, calling `visit` + * for each, and returns the set of function nodes found to be used: `root` + * itself feeds a worklist, and each function it (or a function already on + * the worklist) uses from *its own* reachable statements -- never from a + * dead branch or some other not-yet-reached function's body -- gets queued in + * turn. `outerFunctionsByName` is layered fresh for each scope, so a name + * bound at an inner scope shadows a same-named one further out instead of + * overwriting it globally, and a dead branch that merely mentions a name + * never queues what it holds. + */ +export function walkUsedScopes( + root: Node, + visit: (scope: UsedScope) => void, +): Set { const used = new Set(); const visited = new Set(); const processScope = ( scopeRoot: Node, + scopeFn: FunctionLikeNode | null, outerFunctionsByName: ReadonlyMap, ): void => { if (visited.has(scopeRoot)) return; @@ -463,6 +497,7 @@ export function computeUsedFunctions(root: Node): Set { for (const [name, functions] of functionsHere) { functionsByName.set(name, functions); } + visit({ fn: scopeFn, statements, functionsByName }); const referencedNames = new Set(); for (const statement of statements) { @@ -506,16 +541,24 @@ export function computeUsedFunctions(root: Node): Set { } } - for (const fn of reached) { - used.add(fn); - processScope(fn.body as Node, functionsByName); + for (const reachedFn of reached) { + used.add(reachedFn); + processScope(reachedFn.body as Node, reachedFn, functionsByName); } }; - processScope(root, new Map()); + processScope(root, null, new Map()); return used; } +/** + * The set of function nodes that are actually reachable from `root`. See + * `walkUsedScopes`. + */ +export function computeUsedFunctions(root: Node): Set { + return walkUsedScopes(root, () => {}); +} + /** * A node's `[start, end)` character offsets into the whole source file. * Both engines always populate this -- ESLint forces it on regardless of From c9b9dc7868f2d107418b5f249a443b66f919433c Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sat, 26 Sep 2026 09:46:44 +0900 Subject: [PATCH 04/11] Add a rule for delivery that is never awaited ctx.sendActivity() returns a promise, and an outbox listener that lets it go returns while delivery is still in flight. That usually works out on a long-lived process, but on Cloudflare Workers pending work is dropped once the response is returned, so the activity may never leave. outbox-listener-delivery-required cannot see this: it asks whether a delivery call runs, not whether anything waits for it. The new rule follows the promise of each delivery call up through its parents to where it ends up. It counts as handled when it is awaited, returned, passed to Promise.all() and its siblings, or handed to a waitUntil() method, and void opts a call out. A promise kept in a variable is handled when the variable is mentioned anywhere else, a callback is judged by the call receiving it (forEach() drops what it returns, map() and then() pass it on), and an unawaited call to a local helper that delivers is reported. When it cannot tell where a promise goes, it stays quiet, as outbox-listener-delivery-required does. Deno enables every rule of a plugin as soon as the plugin is listed, so the rule is registered for ESLint and Oxlint only. Its ID stays out of recommendedRuleIds, so the ESLint recommended configuration makes it a warning and strict makes it an error, like every other optional rule. The two outbox rules divide the work between them, and tests pin it down: a listener with a delivery call that can run is never reported by outbox-listener-delivery-required, and one without is never reported by this rule. https://github.com/fedify-dev/fedify/issues/1057 Assisted-by: Claude Code:claude-sonnet-5 --- packages/lint/src/index.ts | 4 + packages/lint/src/lib/const.ts | 1 + packages/lint/src/mod.ts | 3 + packages/lint/src/oxlint.ts | 4 + .../outbox-listener-delivery-not-awaited.ts | 461 +++++ ...-delivery-not-awaited.registration.test.ts | 23 + ...tbox-listener-delivery-not-awaited.test.ts | 1535 +++++++++++++++++ .../outbox-listener-delivery-rules.test.ts | 659 +++++++ 8 files changed, 2690 insertions(+) create mode 100644 packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts create mode 100644 packages/lint/src/tests/outbox-listener-delivery-not-awaited.registration.test.ts create mode 100644 packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts create mode 100644 packages/lint/src/tests/outbox-listener-delivery-rules.test.ts diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index e7a4a1b49..e2456d4dc 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -81,6 +81,9 @@ import { import { eslint as mediaUploaderObjectUriRequired, } from "./rules/media-uploader-object-uri-required.ts"; +import { + eslint as outboxListenerDeliveryNotAwaited, +} from "./rules/outbox-listener-delivery-not-awaited.ts"; import { eslint as outboxListenerDeliveryRequired, } from "./rules/outbox-listener-delivery-required.ts"; @@ -116,6 +119,7 @@ const rules: Record< [RULE_IDS.actorPreferredUsernameRequired]: actorPreferredUsernameRequired, [RULE_IDS.collectionFilteringNotImplemented]: collectionFiltering, [RULE_IDS.outboxListenerDeliveryRequired]: outboxListenerDeliveryRequired, + [RULE_IDS.outboxListenerDeliveryNotAwaited]: outboxListenerDeliveryNotAwaited, [RULE_IDS.mediaUploaderObjectUriRequired]: mediaUploaderObjectUriRequired, [RULE_IDS.mediaUploaderAuthorizationRequired]: mediaUploaderAuthorizationRequired, diff --git a/packages/lint/src/lib/const.ts b/packages/lint/src/lib/const.ts index 7752b0f71..c636b4a88 100644 --- a/packages/lint/src/lib/const.ts +++ b/packages/lint/src/lib/const.ts @@ -159,6 +159,7 @@ export const RULE_IDS = { // Listener rules outboxListenerDeliveryRequired: "outbox-listener-delivery-required", + outboxListenerDeliveryNotAwaited: "outbox-listener-delivery-not-awaited", mediaUploaderObjectUriRequired: "media-uploader-object-uri-required", mediaUploaderAuthorizationRequired: "media-uploader-authorization-required", } as const; diff --git a/packages/lint/src/mod.ts b/packages/lint/src/mod.ts index 5ff51f6e5..6d8bded85 100644 --- a/packages/lint/src/mod.ts +++ b/packages/lint/src/mod.ts @@ -77,6 +77,9 @@ import { deno as outboxListenerDeliveryRequired, } from "./rules/outbox-listener-delivery-required.ts"; +// `outbox-listener-delivery-not-awaited` is deliberately not registered here: +// Deno turns on every rule of a plugin as soon as the plugin is listed, and +// gives the rule no way to stay off until a project asks for it. const plugin: Deno.lint.Plugin = { name: "fedify-lint", rules: { diff --git a/packages/lint/src/oxlint.ts b/packages/lint/src/oxlint.ts index 9f32152c9..dfbb9d7fa 100644 --- a/packages/lint/src/oxlint.ts +++ b/packages/lint/src/oxlint.ts @@ -92,6 +92,9 @@ import { import { eslint as mediaUploaderObjectUriRequired, } from "./rules/media-uploader-object-uri-required.ts"; +import { + eslint as outboxListenerDeliveryNotAwaited, +} from "./rules/outbox-listener-delivery-not-awaited.ts"; import { eslint as outboxListenerDeliveryRequired, } from "./rules/outbox-listener-delivery-required.ts"; @@ -127,6 +130,7 @@ const rules: Record< [RULE_IDS.actorPreferredUsernameRequired]: actorPreferredUsernameRequired, [RULE_IDS.collectionFilteringNotImplemented]: collectionFiltering, [RULE_IDS.outboxListenerDeliveryRequired]: outboxListenerDeliveryRequired, + [RULE_IDS.outboxListenerDeliveryNotAwaited]: outboxListenerDeliveryNotAwaited, [RULE_IDS.mediaUploaderObjectUriRequired]: mediaUploaderObjectUriRequired, [RULE_IDS.mediaUploaderAuthorizationRequired]: mediaUploaderAuthorizationRequired, diff --git a/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts b/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts new file mode 100644 index 000000000..64167ada3 --- /dev/null +++ b/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts @@ -0,0 +1,461 @@ +import type { Rule } from "eslint"; +import { + createOutboxListenerVisitor, + DELIVERY_METHOD_NAMES, + unwrapContextParam, +} from "../lib/outbox-listener.ts"; +import { isNode } from "../lib/pred.ts"; +import { + collectReferencedNames, + type FunctionLikeNode, + getAssignmentTargetName, + isFunctionLikeNode, + type UsedScope, + walkUsedScopes, +} from "../lib/reachability.ts"; +import type { CallExpression, Node } from "../lib/types.ts"; + +const MESSAGE = + "Delivery is not awaited, so the activity may be lost once the handler returns (for example on Cloudflare Workers). Await it, return it, or pass it to waitUntil()."; + +/** + * What becomes of the promise a delivery call returns: + * + * - `"dropped"`: nothing keeps hold of it, which is what the rule reports. + * - `"awaited"`: it reaches an `await`. + * - `"returned"`: it is handed back to whoever called the function. + * - `"handled"`: it is opted out of (`void`), owned by the runtime + * (`waitUntil()`), used somewhere the rule cannot follow, or stored in a + * variable that is mentioned again. The rule stays quiet. + */ +type Fate = "dropped" | "awaited" | "returned" | "handled"; + +/** Wrappers that pass a value through unchanged. */ +const TRANSPARENT_WRAPPERS = new Set([ + "ChainExpression", + "TSAsExpression", + "TSNonNullExpression", + "TSSatisfiesExpression", + "TSTypeAssertion", + "ParenthesizedExpression", +]); + +/** `Promise` methods that settle once every promise they are given has. */ +const PROMISE_COMBINATORS = new Set(["all", "allSettled", "race", "any"]); + +/** Methods on a promise that return a new promise carrying the chain on. */ +const PROMISE_CHAIN_METHODS = new Set(["then", "catch", "finally"]); + +/** + * Methods whose own result is what a callback's return value ends up in, so + * a promise a callback returns is only as safe as that result is. + */ +const CALLBACK_RESULT_METHODS = new Set([ + "map", + "flatMap", + ...PROMISE_CHAIN_METHODS, +]); + +const get = (node: Node | null | undefined, key: string): unknown => + node == null ? undefined : (node as unknown as Record)[key]; + +const asNode = (value: unknown): Node | null => + isNode(value) ? value as Node : null; + +const parentOf = (node: Node): Node | null => asNode(get(node, "parent")); + +function unwrap(node: Node): Node { + let current = node; + while (TRANSPARENT_WRAPPERS.has(current.type)) { + const inner = asNode(get(current, "expression")); + if (inner == null) break; + current = inner; + } + return current; +} + +/** The name a member expression accesses, when it is spelled out plainly. */ +function memberName(node: Node): string | null { + if (node.type !== "MemberExpression") return null; + const property = asNode(get(node, "property")); + if (property == null) return null; + if (get(node, "computed") !== true) { + return property.type === "Identifier" ? property.name : null; + } + if (property.type === "Literal" && typeof property.value === "string") { + return property.value; + } + if (property.type === "TemplateLiteral") { + const quasis = get(property, "quasis") as { value: { cooked?: string } }[]; + const expressions = get(property, "expressions") as unknown[]; + if (expressions.length === 0 && quasis.length === 1) { + return quasis[0].value.cooked ?? null; + } + } + return null; +} + +function visitAll(node: unknown, visitor: (node: Node) => void): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) visitAll(item, visitor); + return; + } + if (!isNode(node)) return; + visitor(node as Node); + const record = node as unknown as Record; + for (const key in record) { + if (key !== "parent") visitAll(record[key], visitor); + } +} + +/** Collects the calls in `node` that run in the same function, not in one nested in it. */ +function collectCalls(node: unknown, out: CallExpression[]): void { + if (node == null || typeof node !== "object") return; + if (Array.isArray(node)) { + for (const item of node) collectCalls(item, out); + return; + } + if (!isNode(node)) return; + const n = node as Node; + if (isFunctionLikeNode(n)) return; + if (n.type === "CallExpression") out.push(n as CallExpression); + const record = n as unknown as Record; + for (const key in record) { + if (key !== "parent") collectCalls(record[key], out); + } +} + +function enclosingFunction(node: Node): FunctionLikeNode | null { + for (let current = parentOf(node); current != null;) { + if (isFunctionLikeNode(current)) return current; + current = parentOf(current); + } + return null; +} + +/** The names an object pattern binds the delivery methods to. */ +function deliveryAliasesOf(pattern: Node, out: Set): void { + if (pattern.type !== "ObjectPattern") return; + for (const prop of get(pattern, "properties") as Node[]) { + if (prop.type !== "Property") continue; + const key = asNode(get(prop, "key")); + const keyName = key?.type === "Identifier" + ? key.name + : key?.type === "Literal" && typeof key.value === "string" + ? key.value + : null; + if (keyName == null || !DELIVERY_METHOD_NAMES.has(keyName)) continue; + const value = asNode(get(prop, "value")); + if (value?.type === "Identifier") out.add(value.name); + else if (value?.type === "AssignmentPattern") { + const left = asNode(get(value, "left")); + if (left?.type === "Identifier") out.add(left.name); + } + } +} + +type DeliveryTargets = { + contextName: string | null; + aliases: Set; +}; + +/** + * Works out how the listener refers to the delivery methods: through its + * context parameter (`ctx.sendActivity`), or through a name bound to one of + * them, either in the parameter (`{ sendActivity }`) or in the body + * (`const { sendActivity } = ctx`, `const send = ctx.sendActivity`). + */ +function findDeliveryTargets(listener: FunctionLikeNode): DeliveryTargets { + const aliases = new Set(); + const contextParam = unwrapContextParam( + listener.params[0] as Node | undefined, + ); + const contextName = contextParam?.type === "Identifier" + ? contextParam.name + : null; + if (contextParam != null) deliveryAliasesOf(contextParam, aliases); + + if (contextName != null) { + visitAll(listener.body, (node) => { + if (node.type !== "VariableDeclarator") return; + const id = asNode(get(node, "id")); + const init = asNode(get(node, "init")); + if (id == null || init == null) return; + const source = unwrap(init); + if (source.type === "Identifier" && source.name === contextName) { + deliveryAliasesOf(id, aliases); + return; + } + if (id.type !== "Identifier" || source.type !== "MemberExpression") { + return; + } + const object = unwrap(asNode(get(source, "object")) ?? source); + const name = memberName(source); + if ( + object.type === "Identifier" && object.name === contextName && + name != null && DELIVERY_METHOD_NAMES.has(name) + ) { + aliases.add(id.name); + } + }); + } + return { contextName, aliases }; +} + +type Analysis = { + listener: FunctionLikeNode; + /** Every name mentioned anywhere in the listener, nested functions included. */ + mentioned: ReadonlySet; +}; + +/** Where the promise a function returns goes, given who calls the function. */ +function returnedFate( + fn: FunctionLikeNode | null, + analysis: Analysis, +): Fate { + if (fn == null) return "handled"; + if (fn === analysis.listener) return "returned"; + const parent = parentOf(fn); + if (parent?.type === "CallExpression") { + // An immediately invoked function returns to the call itself. + if (get(parent, "callee") === fn) return fateOf(parent, analysis); + const args = get(parent, "arguments") as unknown[]; + if (args.includes(fn)) { + const callee = unwrap(asNode(get(parent, "callee")) ?? parent); + const name = memberName(callee); + // `forEach()` discards what its callback returns, which is the one + // callback consumer the rule knows for certain. An unknown one, such + // as `setTimeout()`, may well keep it. + if (name === "forEach") return "dropped"; + if (name != null && CALLBACK_RESULT_METHODS.has(name)) { + return fateOf(parent, analysis); + } + return "handled"; + } + } + // A function declared or held under a name hands the promise to its + // callers, who are judged at each call. + return "returned"; +} + +/** Follows the value of `expression` up through its parents to where it ends up. */ +function fateOf(expression: Node, analysis: Analysis): Fate { + let current = expression; + for (;;) { + const parent = parentOf(current); + if (parent == null) return "handled"; + + if (TRANSPARENT_WRAPPERS.has(parent.type)) { + current = parent; + continue; + } + + switch (parent.type) { + case "AwaitExpression": + return "awaited"; + + case "ReturnStatement": + return returnedFate(enclosingFunction(parent), analysis); + + case "ArrowFunctionExpression": + return get(parent, "body") === current + ? returnedFate(parent as FunctionLikeNode, analysis) + : "handled"; + + case "ExpressionStatement": + return "dropped"; + + // `void` is the way to say a promise is deliberately not awaited, and + // any other operator uses the value somewhere the rule cannot follow. + case "UnaryExpression": + return "handled"; + + case "SequenceExpression": { + const expressions = get(parent, "expressions") as Node[]; + if (expressions[expressions.length - 1] !== current) return "dropped"; + current = parent; + continue; + } + + case "ConditionalExpression": + if (get(parent, "test") === current) return "handled"; + current = parent; + continue; + + case "LogicalExpression": + case "ArrayExpression": + case "SpreadElement": + case "Property": + case "ObjectExpression": + current = parent; + continue; + + case "MemberExpression": { + if (get(parent, "object") !== current) return "handled"; + const call = parentOf(parent); + const name = memberName(parent); + if ( + call?.type === "CallExpression" && get(call, "callee") === parent && + name != null && PROMISE_CHAIN_METHODS.has(name) + ) { + current = call; + continue; + } + return "handled"; + } + + case "CallExpression": { + if (get(parent, "callee") === current) return "handled"; + const callee = unwrap(asNode(get(parent, "callee")) ?? parent); + const name = memberName(callee); + if (name === "waitUntil") return "handled"; + const object = callee.type === "MemberExpression" + ? unwrap(asNode(get(callee, "object")) ?? callee) + : null; + if ( + object?.type === "Identifier" && object.name === "Promise" && + name != null && PROMISE_COMBINATORS.has(name) + ) { + current = parent; + continue; + } + return "handled"; + } + + // A promise kept in a variable is safe when the variable is used + // anywhere else; when nothing ever mentions it again it is forgotten. + case "VariableDeclarator": { + const id = asNode(get(parent, "id")); + if (get(parent, "init") !== current || id?.type !== "Identifier") { + return "handled"; + } + return analysis.mentioned.has(id.name) ? "handled" : "dropped"; + } + + case "AssignmentExpression": { + if (get(parent, "right") !== current) return "handled"; + const name = getAssignmentTargetName( + asNode(get(parent, "left")) ?? parent, + ); + if (name == null) return "handled"; + return analysis.mentioned.has(name) ? "handled" : "dropped"; + } + + default: + return "handled"; + } + } +} + +/** Reports every delivery call in `listener` whose promise is dropped. */ +function checkListener( + listener: FunctionLikeNode, + report: (node: Node) => void, +): void { + const targets = findDeliveryTargets(listener); + if (targets.contextName == null && targets.aliases.size === 0) return; + + const isDeliveryCall = (call: CallExpression): boolean => { + const callee = unwrap(call.callee as Node); + if (callee.type === "MemberExpression") { + const name = memberName(callee); + if (name == null || !DELIVERY_METHOD_NAMES.has(name)) return false; + const object = unwrap(asNode(get(callee, "object")) ?? callee); + return object.type === "Identifier" && + object.name === targets.contextName; + } + return callee.type === "Identifier" && targets.aliases.has(callee.name); + }; + + const mentioned = new Set(); + collectReferencedNames(listener.body, mentioned, true); + const analysis: Analysis = { listener, mentioned }; + + const scopes: UsedScope[] = []; + walkUsedScopes(listener.body as Node, (scope) => scopes.push(scope)); + const callsByScope = scopes.map((scope) => { + const calls: CallExpression[] = []; + for (const statement of scope.statements) collectCalls(statement, calls); + return calls; + }); + + // A local helper that delivers and hands its promise back, by awaiting the + // delivery or by returning it, is only as safe as the way it is called. + const carrying = new Set(); + const carriesDelivery = (call: CallExpression, scope: UsedScope): boolean => { + if (isDeliveryCall(call)) return true; + const callee = unwrap(call.callee as Node); + if (callee.type !== "Identifier") return false; + const helpers = scope.functionsByName.get(callee.name); + return helpers?.some((helper) => carrying.has(helper)) ?? false; + }; + for (let changed = true; changed;) { + changed = false; + scopes.forEach((scope, index) => { + const fn = scope.fn; + if (fn == null || carrying.has(fn)) return; + for (const call of callsByScope[index]) { + if (!carriesDelivery(call, scope)) continue; + const fate = fateOf(call, analysis); + if (fate === "awaited" || fate === "returned") { + carrying.add(fn); + changed = true; + return; + } + } + }); + } + + const reported = new Set(); + scopes.forEach((scope, index) => { + for (const call of callsByScope[index]) { + if (reported.has(call) || !carriesDelivery(call, scope)) continue; + if (fateOf(call, analysis) !== "dropped") continue; + reported.add(call); + report(call); + } + }); +} + +function createRule( + buildReport: Context extends Deno.lint.RuleContext ? { + message: string; + } + : { + messageId: string; + data: { message: string }; + }, +) { + return (context: Context) => + createOutboxListenerVisitor((listener) => + checkListener(listener, (node) => { + (context as { report: (arg: unknown) => void }).report({ + node, + ...buildReport, + }); + }) + ); +} + +export const deno: Deno.lint.Rule = { + create: createRule({ message: MESSAGE }), +}; + +export const eslint: Rule.RuleModule = { + meta: { + type: "suggestion", + docs: { + description: + "Warn when an outbox listener delivers an activity without awaiting it", + }, + schema: [], + messages: { + notAwaited: "{{ message }}", + }, + }, + create: createRule({ + messageId: "notAwaited", + data: { message: MESSAGE }, + }), +}; diff --git a/packages/lint/src/tests/outbox-listener-delivery-not-awaited.registration.test.ts b/packages/lint/src/tests/outbox-listener-delivery-not-awaited.registration.test.ts new file mode 100644 index 000000000..637f44c0e --- /dev/null +++ b/packages/lint/src/tests/outbox-listener-delivery-not-awaited.registration.test.ts @@ -0,0 +1,23 @@ +import { deepStrictEqual, ok } from "node:assert/strict"; +import { test } from "node:test"; +import { plugin } from "../index.ts"; +import { RULE_IDS } from "../lib/const.ts"; +import denoPlugin from "../mod.ts"; +import oxlintPlugin from "../oxlint.ts"; + +const ruleId = RULE_IDS.outboxListenerDeliveryNotAwaited; +const eslintRuleId = `@fedify/lint/${ruleId}`; + +test(`${ruleId}: registered for ESLint, as a warning in recommended and an error in strict`, () => { + ok(plugin.rules != null && ruleId in plugin.rules); + deepStrictEqual(plugin.configs.recommended.rules[eslintRuleId], "warn"); + deepStrictEqual(plugin.configs.strict.rules[eslintRuleId], "error"); +}); + +test(`${ruleId}: registered for Oxlint`, () => { + ok(ruleId in oxlintPlugin.rules); +}); + +test(`${ruleId}: not registered for Deno, which enables every rule of a plugin`, () => { + ok(!(ruleId in denoPlugin.rules)); +}); diff --git a/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts b/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts new file mode 100644 index 000000000..8811325e0 --- /dev/null +++ b/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts @@ -0,0 +1,1535 @@ +import { test } from "node:test"; +import { RULE_IDS } from "../lib/const.ts"; +import lintTest from "../lib/test.ts"; +import * as rule from "../rules/outbox-listener-delivery-not-awaited.ts"; + +const ruleName = RULE_IDS.outboxListenerDeliveryNotAwaited; + +test( + `${ruleName}: ✅ Good - awaited sendActivity call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited forwardActivity call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await ctx.forwardActivity(sender, [], { skipIfUnsigned: true }); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery returned from the listener`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + return ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited call through an optional chain`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await ctx?.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited call through bracket notation`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await ctx["sendActivity"](sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited call through a type assertion`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await (ctx as any).sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited call inside a loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + for (const target of [inbox]) { + await ctx.sendActivity(sender, target, activity); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited call inside try/catch`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + try { + await ctx.sendActivity(sender, inbox, activity); + } catch (error) { + console.error(error); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited call in a conditional branch`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (activity.id != null) { + await ctx.sendActivity(sender, inbox, activity); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - Promise.all over map`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - Promise.all returned`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + return Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - Promise.allSettled over map`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.allSettled(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promises spread into Promise.all`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.all([...inboxes.map((target) => ctx.sendActivity(sender, target, activity)), ...others.map((target) => + ctx.sendActivity(sender, target, activity) + )]); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - Promise.all result kept in a variable`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const results = await Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + console.log(results); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - map result kept and awaited later`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const promises = inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + await Promise.all(promises); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promise handed to waitUntil`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + executionCtx.waitUntil(ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promise handed to a nested waitUntil`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + c.executionCtx.waitUntil(ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery opted out with void`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + void ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited chain ending in catch`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await ctx.sendActivity(sender, inbox, activity).catch(console.error); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited chain through then`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await ctx.sendActivity(sender, inbox, activity).then(() => console.log("sent")); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited then callback returning a delivery`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.resolve().then(() => ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promise stored and awaited later`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const pending = ctx.sendActivity(sender, inbox, activity); + await pending; + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promise stored and used in a nested callback`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const pending = ctx.sendActivity(sender, inbox, activity); + setTimeout(() => pending.then(console.log), 0); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promise stored in an object that is used`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const jobs = { pending: ctx.sendActivity(sender, inbox, activity) }; + await jobs.pending; + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promise assigned and awaited later`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + let pending; + pending = ctx.sendActivity(sender, inbox, activity); + await pending; + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper that awaits delivery, awaited by the caller`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + } + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper that returns delivery, awaited by the caller`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => ctx.sendActivity(sender, inbox, activity); + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper that returns delivery, returned by the listener`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => ctx.sendActivity(sender, inbox, activity); + return deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - destructured delivery method, awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const { sendActivity } = ctx; + await sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery method taken from ctx, awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const send = ctx.sendActivity; + await send(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - dead branch with an unawaited call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (false) { + ctx.sendActivity(sender, inbox, activity); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - unreachable code after return`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + return; + ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - unused helper with an unawaited call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + function deliver() { + ctx.sendActivity(sender, inbox, activity); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - callback handed to setTimeout`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + setTimeout(() => ctx.sendActivity(sender, inbox, activity), 0); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - callback handed to queue.push`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const queue = []; + queue.push(() => ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - listener that delivers nothing`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + console.log(ctx.identifier, activity.id?.href); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - arrow listener with an expression body`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, (ctx, activity) => + ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + )); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery method destructured in the parameters`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async ({ sendActivity }, activity) => { + await sendActivity( + { identifier: "alice" }, + new URL("https://example.com/inbox"), + activity, + ); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - named listener that awaits delivery`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +async function handler(ctx, activity) { + await ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); +} + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, handler); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - unawaited call in a helper declared outside the listener`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +function deliverIt(ctx, activity) { + ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity); +} + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + deliverIt(ctx, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - bare sendActivity call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare forwardActivity call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.forwardActivity(sender, [], { skipIfUnsigned: true }); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - discarded chain ending in catch`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity).catch(console.error); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - discarded chain through then`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity).then(() => console.log("sent")); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - discarded chain ending in finally`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity).finally(() => console.log("done")); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - discarded then callback returning a delivery`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.resolve().then(() => ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - promise stored and never used`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const pending = ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - promise assigned and never used`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + let pending; + pending = ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - promise stored in an object that is never used`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const jobs = { pending: ctx.sendActivity(sender, inbox, activity) }; + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - callback passed to forEach`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + inboxes.forEach((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - map result dropped`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - map result kept but never used`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const promises = inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - Promise.all that is never awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call inside an async callback`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.all(inboxes.map(async (target) => { + ctx.sendActivity(sender, target, activity); + })); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call inside a try block`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + try { + ctx.sendActivity(sender, inbox, activity); + } catch (error) { + console.error(error); + } + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call inside a loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + for (const target of [inbox]) { + ctx.sendActivity(sender, target, activity); + } + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call behind a logical and`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + activity.id != null && ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call in a conditional expression`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + activity.id != null ? ctx.sendActivity(sender, inbox, activity) : null; + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call in a sequence expression`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + (ctx.sendActivity(sender, inbox, activity), console.log("sent")); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call in an immediately invoked function`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + (async () => { + return ctx.sendActivity(sender, inbox, activity); + })(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call through an optional chain`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx?.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call through bracket notation`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx["sendActivity"](sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call through a type assertion`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + (ctx as any).sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call to a destructured delivery method`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const { sendActivity } = ctx; + sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call to a delivery method taken from ctx`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const send = ctx.sendActivity; + send(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - helper that awaits delivery, called without await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + } + deliver(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - helper that returns delivery, called without await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => ctx.sendActivity(sender, inbox, activity); + deliver(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - helper that calls a delivering helper, called without await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + } + async function outer() { + await deliver(); + } + outer(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - helper with a bare call inside it, called with await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + ctx.sendActivity(sender, inbox, activity); + } + await deliver(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery method destructured in the parameters, called without await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async ({ sendActivity }, activity) => { + sendActivity( + { identifier: "alice" }, + new URL("https://example.com/inbox"), + activity, + ); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - named listener with a bare call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +async function handler(ctx, activity) { + ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); +} + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, handler); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ✅ Good - non-federation object`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +const fakeFederation = { + setOutboxListeners() { + return { + on() { + return this; + }, + }; + }, +}; + +fakeFederation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity); + }); +`, + rule, + ruleName, + federationSetup: "", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call in a class method`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + class Sender { + deliver() { + ctx.sendActivity(sender, inbox, activity); + } + } + new Sender().deliver(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - bare call in an object method`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { + deliver() { + ctx.sendActivity(sender, inbox, activity); + }, + }; + handlers.deliver(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); diff --git a/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts b/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts new file mode 100644 index 000000000..15588dd5d --- /dev/null +++ b/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts @@ -0,0 +1,659 @@ +import { test } from "node:test"; +import { RULE_IDS } from "../lib/const.ts"; +import lintTest from "../lib/test.ts"; +import * as notAwaited from "../rules/outbox-listener-delivery-not-awaited.ts"; +import * as required from "../rules/outbox-listener-delivery-required.ts"; + +// The two rules divide the work: outbox-listener-delivery-required reports a +// listener with no delivery call that can run, and +// outbox-listener-delivery-not-awaited reports a delivery call that can run +// but is not awaited. Neither should have anything to say about what the +// other one reports. + +const REQUIRED_MESSAGE = "Outbox listeners should deliver posted activities"; +const NOT_AWAITED_MESSAGE = "Delivery is not awaited"; + +// Every listener the not-awaited rule reports has a delivery call. +const UNAWAITED: [title: string, code: string][] = [ + [ + "bare sendActivity call", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "bare forwardActivity call", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.forwardActivity(sender, [], { skipIfUnsigned: true }); + }); +`, + ], + [ + "discarded chain ending in catch", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity).catch(console.error); + }); +`, + ], + [ + "discarded chain through then", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity).then(() => console.log("sent")); + }); +`, + ], + [ + "discarded chain ending in finally", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx.sendActivity(sender, inbox, activity).finally(() => console.log("done")); + }); +`, + ], + [ + "discarded then callback returning a delivery", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.resolve().then(() => ctx.sendActivity(sender, inbox, activity)); + }); +`, + ], + [ + "promise stored and never used", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const pending = ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "promise assigned and never used", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + let pending; + pending = ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "promise stored in an object that is never used", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const jobs = { pending: ctx.sendActivity(sender, inbox, activity) }; + }); +`, + ], + [ + "callback passed to forEach", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + inboxes.forEach((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + ], + [ + "map result dropped", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + ], + [ + "map result kept but never used", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const promises = inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + ], + [ + "Promise.all that is never awaited", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + }); +`, + ], + [ + "bare call inside an async callback", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.all(inboxes.map(async (target) => { + ctx.sendActivity(sender, target, activity); + })); + }); +`, + ], + [ + "bare call inside a try block", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + try { + ctx.sendActivity(sender, inbox, activity); + } catch (error) { + console.error(error); + } + }); +`, + ], + [ + "bare call inside a loop", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + for (const target of [inbox]) { + ctx.sendActivity(sender, target, activity); + } + }); +`, + ], + [ + "bare call behind a logical and", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + activity.id != null && ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "bare call in a conditional expression", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + activity.id != null ? ctx.sendActivity(sender, inbox, activity) : null; + }); +`, + ], + [ + "bare call in a sequence expression", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + (ctx.sendActivity(sender, inbox, activity), console.log("sent")); + }); +`, + ], + [ + "bare call in an immediately invoked function", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + (async () => { + return ctx.sendActivity(sender, inbox, activity); + })(); + }); +`, + ], + [ + "bare call through an optional chain", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx?.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "bare call through bracket notation", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx["sendActivity"](sender, inbox, activity); + }); +`, + ], + [ + "bare call through a type assertion", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + (ctx as any).sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "bare call to a destructured delivery method", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const { sendActivity } = ctx; + sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "bare call to a delivery method taken from ctx", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const send = ctx.sendActivity; + send(sender, inbox, activity); + }); +`, + ], + [ + "helper that awaits delivery, called without await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + } + deliver(); + }); +`, + ], + [ + "helper that returns delivery, called without await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => ctx.sendActivity(sender, inbox, activity); + deliver(); + }); +`, + ], + [ + "helper that calls a delivering helper, called without await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + } + async function outer() { + await deliver(); + } + outer(); + }); +`, + ], + [ + "helper with a bare call inside it, called with await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + ctx.sendActivity(sender, inbox, activity); + } + await deliver(); + }); +`, + ], + [ + "delivery method destructured in the parameters, called without await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async ({ sendActivity }, activity) => { + sendActivity( + { identifier: "alice" }, + new URL("https://example.com/inbox"), + activity, + ); + }); +`, + ], + [ + "named listener with a bare call", + ` +import { Activity } from "@fedify/vocab"; + +async function handler(ctx, activity) { + ctx.sendActivity( + { identifier: ctx.identifier }, + new URL("https://example.com/inbox"), + activity, + ); +} + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, handler); +`, + ], + [ + "bare call in a class method", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + class Sender { + deliver() { + ctx.sendActivity(sender, inbox, activity); + } + } + new Sender().deliver(); + }); +`, + ], + [ + "bare call in an object method", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { + deliver() { + ctx.sendActivity(sender, inbox, activity); + }, + }; + handlers.deliver(); + }); +`, + ], +]; + +// A listener with no delivery call that can run is the required rule's business. +const NO_DELIVERY: [title: string, code: string][] = [ + [ + "a listener that delivers nothing", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + console.log(ctx.identifier); + }); +`, + ], + [ + "a delivery call only in a dead branch", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (false) { + ctx.sendActivity(sender, inbox, activity); + } + }); +`, + ], + [ + "a delivery call after a return", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + return; + ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "a delivery call after a throw", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + throw new Error("stop"); + ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "a delivery call only in a helper nothing uses", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + function deliver() { + ctx.sendActivity(sender, inbox, activity); + } + }); +`, + ], + [ + "a delivery call only in an object nothing uses", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const handlers = { deliver: () => ctx.sendActivity(sender, inbox, activity) }; + }); +`, + ], +]; + +for (const [title, code] of UNAWAITED) { + test( + `outbox-listener rules: ✅ delivery-required stays quiet on ${title}`, + lintTest({ + code, + rule: required, + ruleName: RULE_IDS.outboxListenerDeliveryRequired, + }), + ); +} + +for (const [title, code] of NO_DELIVERY) { + test( + `outbox-listener rules: ❌ delivery-required reports ${title}`, + lintTest({ + code, + rule: required, + ruleName: RULE_IDS.outboxListenerDeliveryRequired, + expectedError: REQUIRED_MESSAGE, + }), + ); + test( + `outbox-listener rules: ✅ delivery-not-awaited stays quiet on ${title}`, + lintTest({ + code, + rule: notAwaited, + ruleName: RULE_IDS.outboxListenerDeliveryNotAwaited, + }), + ); +} + +for (const [title, code] of UNAWAITED.slice(0, 1)) { + test( + `outbox-listener rules: ❌ delivery-not-awaited reports ${title}`, + lintTest({ + code, + rule: notAwaited, + ruleName: RULE_IDS.outboxListenerDeliveryNotAwaited, + expectedError: NOT_AWAITED_MESSAGE, + }), + ); +} From 8ddde7bd6f7b7e98043618c597c9f344faaf936b Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sat, 26 Sep 2026 09:46:53 +0900 Subject: [PATCH 05/11] Document the delivery-not-awaited rule Add a manual section that says what the rule counts as handled and what it reports, why an unawaited delivery can be lost, and where the rule is available, and link it from outbox-listener-delivery-required, which pointed at the issue instead. List the rule in the package README. https://github.com/fedify-dev/fedify/issues/1057 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 13 ++ changes.d/lint/outbox-delivery-not-awaited.md | 15 +++ docs/manual/lint.md | 111 +++++++++++++++++- packages/lint/README.md | 3 + 4 files changed, 138 insertions(+), 4 deletions(-) create mode 100644 changes.d/lint/outbox-delivery-not-awaited.md diff --git a/CHANGES.md b/CHANGES.md index 4a1160a8d..6933e9e0c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -283,6 +283,18 @@ To be released. when an actor dispatcher's return value does not include a `preferredUsername` property. [[#895], [#1022] by Jae-Hyuk-Jang\] + - Added the `outbox-listener-delivery-not-awaited` rule to `@fedify/lint`. + It reports an outbox listener that calls `ctx.sendActivity()` or + `ctx.forwardActivity()` and drops the returned promise, so that the + activity may never leave on a runtime such as Cloudflare Workers, which + discards pending work once the response is returned. A call counts as + handled when its promise is awaited, returned, passed to `Promise.all()` + and its siblings, or handed to `waitUntil()`, and `void` opts a call out. + The ESLint `recommended` configuration enables the rule as a warning and + `strict` as an error, and Oxlint users enable it by name. It is not + available in Deno Lint, which turns on every rule of a plugin at once. + [[#1057] by Jae-Hyuk-Jang\] + - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide whether a `ctx.sendActivity()`/`ctx.forwardActivity()` call actually runs, instead of scanning the listener's source as a flat block of text. @@ -298,6 +310,7 @@ To be released. [#900]: https://github.com/fedify-dev/fedify/issues/900 [#1022]: https://github.com/fedify-dev/fedify/pull/1022 [#1050]: https://github.com/fedify-dev/fedify/pull/1050 +[#1057]: https://github.com/fedify-dev/fedify/issues/1057 ### @fedify/mysql diff --git a/changes.d/lint/outbox-delivery-not-awaited.md b/changes.d/lint/outbox-delivery-not-awaited.md new file mode 100644 index 000000000..dfdb8093a --- /dev/null +++ b/changes.d/lint/outbox-delivery-not-awaited.md @@ -0,0 +1,15 @@ +--- +links: + '#1057': https://github.com/fedify-dev/fedify/issues/1057 +--- + - Added the `outbox-listener-delivery-not-awaited` rule to `@fedify/lint`. + It reports an outbox listener that calls `ctx.sendActivity()` or + `ctx.forwardActivity()` and drops the returned promise, so that the + activity may never leave on a runtime such as Cloudflare Workers, which + discards pending work once the response is returned. A call counts as + handled when its promise is awaited, returned, passed to `Promise.all()` + and its siblings, or handed to `waitUntil()`, and `void` opts a call out. + The ESLint `recommended` configuration enables the rule as a warning and + `strict` as an error, and Oxlint users enable it by name. It is not + available in Deno Lint, which turns on every rule of a plugin at once. + [[#1057] by Jae-Hyuk-Jang] diff --git a/docs/manual/lint.md b/docs/manual/lint.md index 506219229..66a242808 100644 --- a/docs/manual/lint.md +++ b/docs/manual/lint.md @@ -781,9 +781,9 @@ explicit delivery path, and that path must actually run. The rule checks that a delivery call exists and can run, not that the delivery completes, so a listener it accepts is not guaranteed to federate. A delivery -call that is never awaited is not reported; -[#1057] tracks a rule for -that. +call that is never awaited is not reported here; the opt-in +[`outbox-listener-delivery-not-awaited`](#outbox-listener-delivery-not-awaited) +rule checks for that. ~~~~ typescript twoslash // @noErrors: 2345 @@ -859,7 +859,110 @@ federation }); ~~~~ -[#1057]: https://github.com/fedify-dev/fedify/issues/1057 +### `outbox-listener-delivery-not-awaited` + +Warns when an outbox listener calls `ctx.sendActivity()` or +`ctx.forwardActivity()` and lets the returned promise go without waiting for it. + +::: info +This rule is available in ESLint and Oxlint, but not in Deno Lint: Deno turns on +every rule of a plugin as soon as the plugin is listed, and gives a project no +way to keep one off until it asks for it. In ESLint, the *recommended* +configuration enables it as a warning and *strict* as an error. In Oxlint, +enable it by name. +::: + +**When this rule applies:** +You've registered an outbox listener with `setOutboxListeners()`, and it calls +`ctx.sendActivity()` or `ctx.forwardActivity()` in a way that drops the +returned promise. The rule follows the promise from the call to where it ends +up. A call counts as handled when its promise is awaited, returned, passed to +`Promise.all()`, `Promise.allSettled()`, `Promise.race()` or `Promise.any()`, +or handed to a method named `waitUntil()`. A call is reported when its promise +is discarded, including when it is only kept in a variable that nothing else +uses, or returned from a callback that is passed to `forEach()`. + +When it cannot tell where a promise goes, the rule stays quiet. In practice: + + - `void ctx.sendActivity(...)` is read as a deliberate choice and is not + reported. A discarded `.catch()`, `.then()` or `.finally()` chain is + reported, since a `.catch()` handles the error but does not wait. + - A local helper that delivers is judged by how it is called: `deliver();` is + reported when `deliver()` awaits or returns a delivery, and + `await deliver();` is not. + - A promise passed to a function the rule does not know, such as + `queue.push(...)` or `setTimeout(...)`, is left alone, since the rule cannot + tell what that function does with it. + - As in `outbox-listener-delivery-required`, the rule reads only the listener + body. A delivery call in a helper that is declared outside the listener, + or in another module, is not seen. + +**Why it matters:** +`ctx.sendActivity()` returns a promise. A listener that calls it without +waiting hands that promise to nobody, and the handler can return while delivery +is still in flight. On a long-lived Node.js or Deno process this usually works +out. On Cloudflare Workers, which Fedify supports through `@fedify/cfworkers`, +pending work is dropped once the response is returned, so the activity may never +leave. Every `sendActivity()` example in [*Sending activities*](./send.md) +awaits the call. + +This rule is separate from +[`outbox-listener-delivery-required`](#outbox-listener-delivery-required), +which asks whether a delivery call exists and can run, not whether anything +waits for it. + +~~~~ typescript twoslash +// @noErrors: 2345 +import { createFederation } from "@fedify/fedify"; +import { Activity } from "@fedify/vocab"; +import type { Recipient } from "@fedify/vocab"; +const federation = createFederation({ kv: null as any }); +declare const recipients: Recipient[]; +declare const executionCtx: { waitUntil(promise: Promise): void }; +// ---cut-before--- +// ❌ Bad: The promise is dropped, so the activity can be lost +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity); + }); + +// ❌ Bad: forEach() discards what its callback returns +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + recipients.forEach((recipient) => + ctx.sendActivity({ identifier: ctx.identifier }, recipient, activity) + ); + }); + +// ✅ Good: The delivery is awaited +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + await ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity); + }); + +// ✅ Good: Every delivery is awaited together +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + await Promise.all( + recipients.map((recipient) => + ctx.sendActivity({ identifier: ctx.identifier }, recipient, activity) + ), + ); + }); + +// ✅ Good: The runtime keeps the work alive after the response is returned +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + executionCtx.waitUntil( + ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity), + ); + }); +~~~~ ### `media-uploader-object-uri-required` diff --git a/packages/lint/README.md b/packages/lint/README.md index 40412b464..5eac98c4b 100644 --- a/packages/lint/README.md +++ b/packages/lint/README.md @@ -140,6 +140,9 @@ federation code: callback does not derive its return value from `getObjectUri` - **`media-uploader-authorization-required`**: Warns when `setMediaUploader` is registered without an `authorize` hook + - **`outbox-listener-delivery-not-awaited`**: Warns when an outbox listener + calls `sendActivity` or `forwardActivity` and does not wait for the result + (ESLint and Oxlint only) Installation From d381532db2c0d45cdf553353eedbf2cd7cf37b3d Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sat, 26 Sep 2026 09:59:58 +0900 Subject: [PATCH 06/11] Add PR credit to the delivery-not-awaited changelog fragment Link the pull request from the changelog entry, now that it is open, as the changelog convention asks. https://github.com/fedify-dev/fedify/pull/1067 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 3 ++- changes.d/lint/outbox-delivery-not-awaited.md | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 6933e9e0c..1f959d41d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -293,7 +293,7 @@ To be released. The ESLint `recommended` configuration enables the rule as a warning and `strict` as an error, and Oxlint users enable it by name. It is not available in Deno Lint, which turns on every rule of a plugin at once. - [[#1057] by Jae-Hyuk-Jang\] + [[#1057], [#1067] by Jae-Hyuk-Jang\] - Changed `outbox-listener-delivery-required` (`@fedify/lint`) to decide whether a `ctx.sendActivity()`/`ctx.forwardActivity()` call actually @@ -311,6 +311,7 @@ To be released. [#1022]: https://github.com/fedify-dev/fedify/pull/1022 [#1050]: https://github.com/fedify-dev/fedify/pull/1050 [#1057]: https://github.com/fedify-dev/fedify/issues/1057 +[#1067]: https://github.com/fedify-dev/fedify/pull/1067 ### @fedify/mysql diff --git a/changes.d/lint/outbox-delivery-not-awaited.md b/changes.d/lint/outbox-delivery-not-awaited.md index dfdb8093a..79ff9f323 100644 --- a/changes.d/lint/outbox-delivery-not-awaited.md +++ b/changes.d/lint/outbox-delivery-not-awaited.md @@ -1,6 +1,7 @@ --- links: '#1057': https://github.com/fedify-dev/fedify/issues/1057 + '#1067': https://github.com/fedify-dev/fedify/pull/1067 --- - Added the `outbox-listener-delivery-not-awaited` rule to `@fedify/lint`. It reports an outbox listener that calls `ctx.sendActivity()` or @@ -12,4 +13,4 @@ links: The ESLint `recommended` configuration enables the rule as a warning and `strict` as an error, and Oxlint users enable it by name. It is not available in Deno Lint, which turns on every rule of a plugin at once. - [[#1057] by Jae-Hyuk-Jang] + [[#1057], [#1067] by Jae-Hyuk-Jang] From bf25197208a875d925feec7b55282659d7ed3c26 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sat, 26 Sep 2026 15:33:23 +0900 Subject: [PATCH 07/11] Follow arrays of promises and async callbacks Review of the new rule found delivery promises left in flight that it let through. An array of promises, such as what map() returns, waits for nothing on its own, yet awaiting or returning it was accepted as if it were one promise. Follow the shape of the value as well as where it goes: an array counts as handled only once it reaches Promise.all() or one of its siblings, or a variable that is mentioned again, and awaiting it or returning it from the listener is reported. An array returned from a helper is left alone, since the helper's caller may pass it to Promise.all(). fateOf() now also says which function the promise ended up in, so a helper that returns or awaits Promise.all() over a map is what carries the delivery, not the map callback inside it, and calling that helper without await is reported. A function that awaits a delivery is judged by where its own promise goes, as a returning callback already was: an async callback given to forEach() is dropped, an immediately invoked one is as safe as the call, one given to map() is as safe as the array, and a function handed over by name, as in forEach(deliver), is judged the same way. A callback given to an unknown function is still left alone. Also treat every unary operator except void as dropping the promise, collect delivery aliases only from the scopes that can run instead of the whole listener, and read a template literal member name from Deno.lint's AST, where reading it as ESTree threw. Promise.race() and Promise.any() stay accepted as a deliberate choice, and a dropped one is pinned by a test. https://github.com/fedify-dev/fedify/pull/1067#pullrequestreview-5323966182 Assisted-by: Claude Code:claude-sonnet-5 --- .../outbox-listener-delivery-not-awaited.ts | 356 +++++--- ...tbox-listener-delivery-not-awaited.test.ts | 848 ++++++++++++++++++ .../outbox-listener-delivery-rules.test.ts | 318 +++++++ 3 files changed, 1401 insertions(+), 121 deletions(-) diff --git a/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts b/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts index 64167ada3..6062b12a2 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts @@ -19,17 +19,34 @@ const MESSAGE = "Delivery is not awaited, so the activity may be lost once the handler returns (for example on Cloudflare Workers). Await it, return it, or pass it to waitUntil()."; /** - * What becomes of the promise a delivery call returns: + * What becomes of a promise the rule is following: * * - `"dropped"`: nothing keeps hold of it, which is what the rule reports. * - `"awaited"`: it reaches an `await`. * - `"returned"`: it is handed back to whoever called the function. - * - `"handled"`: it is opted out of (`void`), owned by the runtime - * (`waitUntil()`), used somewhere the rule cannot follow, or stored in a - * variable that is mentioned again. The rule stays quiet. + * - `"handled"`: it is opted out of (`void`, `Promise.race()`), owned by the + * runtime (`waitUntil()`), used somewhere the rule cannot follow, or + * stored in a variable that is mentioned again. The rule stays quiet. */ type Fate = "dropped" | "awaited" | "returned" | "handled"; +/** + * A fate, with the function the promise ended up in when that is an `await` + * or a `return`: the function that now carries the promise on to its own + * callers. + */ +type Outcome = { fate: Fate; owner: FunctionLikeNode | null }; + +const HANDLED: Outcome = { fate: "handled", owner: null }; +const DROPPED: Outcome = { fate: "dropped", owner: null }; + +/** + * What the value being followed is. An array of promises, such as what + * `map()` returns, waits for nothing until it reaches `Promise.all()` or one + * of its siblings: awaiting or returning it leaves every promise in flight. + */ +type Shape = "promise" | "promises"; + /** Wrappers that pass a value through unchanged. */ const TRANSPARENT_WRAPPERS = new Set([ "ChainExpression", @@ -40,22 +57,17 @@ const TRANSPARENT_WRAPPERS = new Set([ "ParenthesizedExpression", ]); -/** `Promise` methods that settle once every promise they are given has. */ +/** + * `Promise` methods that turn promises into one promise. `race()` and + * `any()` settle as soon as one input does, which is accepted as a + * deliberate choice to stop waiting, like `void`. A `race()` or `any()` + * that is itself dropped is still reported. + */ const PROMISE_COMBINATORS = new Set(["all", "allSettled", "race", "any"]); /** Methods on a promise that return a new promise carrying the chain on. */ const PROMISE_CHAIN_METHODS = new Set(["then", "catch", "finally"]); -/** - * Methods whose own result is what a callback's return value ends up in, so - * a promise a callback returns is only as safe as that result is. - */ -const CALLBACK_RESULT_METHODS = new Set([ - "map", - "flatMap", - ...PROMISE_CHAIN_METHODS, -]); - const get = (node: Node | null | undefined, key: string): unknown => node == null ? undefined : (node as unknown as Record)[key]; @@ -86,44 +98,44 @@ function memberName(node: Node): string | null { return property.value; } if (property.type === "TemplateLiteral") { - const quasis = get(property, "quasis") as { value: { cooked?: string } }[]; + // ESTree keeps the text under `value`, and Deno.lint exposes it directly. + const quasis = get(property, "quasis") as { + cooked?: string; + value?: { cooked?: string }; + }[]; const expressions = get(property, "expressions") as unknown[]; if (expressions.length === 0 && quasis.length === 1) { - return quasis[0].value.cooked ?? null; + return quasis[0].cooked ?? quasis[0].value?.cooked ?? null; } } return null; } -function visitAll(node: unknown, visitor: (node: Node) => void): void { +/** Visits every node in `node`, stopping at a nested function unless asked to cross it. */ +function visitAll( + node: unknown, + visitor: (node: Node) => void, + crossFunctions = true, +): void { if (node == null || typeof node !== "object") return; if (Array.isArray(node)) { - for (const item of node) visitAll(item, visitor); + for (const item of node) visitAll(item, visitor, crossFunctions); return; } if (!isNode(node)) return; + if (!crossFunctions && isFunctionLikeNode(node as Node)) return; visitor(node as Node); const record = node as unknown as Record; for (const key in record) { - if (key !== "parent") visitAll(record[key], visitor); + if (key !== "parent") visitAll(record[key], visitor, crossFunctions); } } /** Collects the calls in `node` that run in the same function, not in one nested in it. */ function collectCalls(node: unknown, out: CallExpression[]): void { - if (node == null || typeof node !== "object") return; - if (Array.isArray(node)) { - for (const item of node) collectCalls(item, out); - return; - } - if (!isNode(node)) return; - const n = node as Node; - if (isFunctionLikeNode(n)) return; - if (n.type === "CallExpression") out.push(n as CallExpression); - const record = n as unknown as Record; - for (const key in record) { - if (key !== "parent") collectCalls(record[key], out); - } + visitAll(node, (n) => { + if (n.type === "CallExpression") out.push(n as CallExpression); + }, false); } function enclosingFunction(node: Node): FunctionLikeNode | null { @@ -163,10 +175,14 @@ type DeliveryTargets = { /** * Works out how the listener refers to the delivery methods: through its * context parameter (`ctx.sendActivity`), or through a name bound to one of - * them, either in the parameter (`{ sendActivity }`) or in the body - * (`const { sendActivity } = ctx`, `const send = ctx.sendActivity`). + * them, either in the parameter (`{ sendActivity }`) or in code that can run + * (`const { sendActivity } = ctx`, `const send = ctx.sendActivity`). A name + * bound in a helper nothing uses, or in a dead branch, does not count. */ -function findDeliveryTargets(listener: FunctionLikeNode): DeliveryTargets { +function findDeliveryTargets( + listener: FunctionLikeNode, + scopes: readonly UsedScope[], +): DeliveryTargets { const aliases = new Set(); const contextParam = unwrapContextParam( listener.params[0] as Node | undefined, @@ -177,28 +193,32 @@ function findDeliveryTargets(listener: FunctionLikeNode): DeliveryTargets { if (contextParam != null) deliveryAliasesOf(contextParam, aliases); if (contextName != null) { - visitAll(listener.body, (node) => { - if (node.type !== "VariableDeclarator") return; - const id = asNode(get(node, "id")); - const init = asNode(get(node, "init")); - if (id == null || init == null) return; - const source = unwrap(init); - if (source.type === "Identifier" && source.name === contextName) { - deliveryAliasesOf(id, aliases); - return; - } - if (id.type !== "Identifier" || source.type !== "MemberExpression") { - return; + for (const scope of scopes) { + for (const statement of scope.statements) { + visitAll(statement, (node) => { + if (node.type !== "VariableDeclarator") return; + const id = asNode(get(node, "id")); + const init = asNode(get(node, "init")); + if (id == null || init == null) return; + const source = unwrap(init); + if (source.type === "Identifier" && source.name === contextName) { + deliveryAliasesOf(id, aliases); + return; + } + if (id.type !== "Identifier" || source.type !== "MemberExpression") { + return; + } + const object = unwrap(asNode(get(source, "object")) ?? source); + const name = memberName(source); + if ( + object.type === "Identifier" && object.name === contextName && + name != null && DELIVERY_METHOD_NAMES.has(name) + ) { + aliases.add(id.name); + } + }, false); } - const object = unwrap(asNode(get(source, "object")) ?? source); - const name = memberName(source); - if ( - object.type === "Identifier" && object.name === contextName && - name != null && DELIVERY_METHOD_NAMES.has(name) - ) { - aliases.add(id.name); - } - }); + } } return { contextName, aliases }; } @@ -209,42 +229,71 @@ type Analysis = { mentioned: ReadonlySet; }; +/** + * Where the value a callback returns goes, judged by the call that receives + * the callback. + */ +function callbackResultOutcome(call: Node, analysis: Analysis): Outcome { + const callee = unwrap(asNode(get(call, "callee")) ?? call); + const name = memberName(callee); + // `forEach()` discards what its callback returns, which is the one callback + // consumer the rule knows for certain. An unknown one, such as + // `setTimeout()`, may well keep it. + if (name === "forEach") return DROPPED; + if (name === "map" || name === "flatMap") { + return fateOf(call, analysis, "promises"); + } + if (name != null && PROMISE_CHAIN_METHODS.has(name)) { + return fateOf(call, analysis, "promise"); + } + return HANDLED; +} + /** Where the promise a function returns goes, given who calls the function. */ -function returnedFate( +function returnedOutcome( fn: FunctionLikeNode | null, analysis: Analysis, -): Fate { - if (fn == null) return "handled"; - if (fn === analysis.listener) return "returned"; +): Outcome { + if (fn == null) return HANDLED; + if (fn === analysis.listener) return { fate: "returned", owner: fn }; const parent = parentOf(fn); if (parent?.type === "CallExpression") { // An immediately invoked function returns to the call itself. - if (get(parent, "callee") === fn) return fateOf(parent, analysis); - const args = get(parent, "arguments") as unknown[]; - if (args.includes(fn)) { - const callee = unwrap(asNode(get(parent, "callee")) ?? parent); - const name = memberName(callee); - // `forEach()` discards what its callback returns, which is the one - // callback consumer the rule knows for certain. An unknown one, such - // as `setTimeout()`, may well keep it. - if (name === "forEach") return "dropped"; - if (name != null && CALLBACK_RESULT_METHODS.has(name)) { - return fateOf(parent, analysis); - } - return "handled"; + if (get(parent, "callee") === fn) { + return fateOf(parent, analysis, "promise"); + } + if ((get(parent, "arguments") as unknown[]).includes(fn)) { + return callbackResultOutcome(parent, analysis); } } // A function declared or held under a name hands the promise to its // callers, who are judged at each call. - return "returned"; + return { fate: "returned", owner: fn }; +} + +/** + * Where an array of promises goes when it is returned. The listener's caller + * does not wait for what is inside it. Any other function hands it to its + * own callers, who may well pass it to `Promise.all()`. + */ +function returnedArrayOutcome( + fn: FunctionLikeNode | null, + analysis: Analysis, +): Outcome { + return fn === analysis.listener ? DROPPED : HANDLED; } /** Follows the value of `expression` up through its parents to where it ends up. */ -function fateOf(expression: Node, analysis: Analysis): Fate { +function fateOf( + expression: Node, + analysis: Analysis, + shape: Shape = "promise", +): Outcome { let current = expression; + let currentShape = shape; for (;;) { const parent = parentOf(current); - if (parent == null) return "handled"; + if (parent == null) return HANDLED; if (TRANSPARENT_WRAPPERS.has(parent.type)) { current = parent; @@ -252,64 +301,86 @@ function fateOf(expression: Node, analysis: Analysis): Fate { } switch (parent.type) { + // Awaiting an array of promises waits for none of them. case "AwaitExpression": - return "awaited"; + return currentShape === "promise" + ? { fate: "awaited", owner: enclosingFunction(parent) } + : DROPPED; case "ReturnStatement": - return returnedFate(enclosingFunction(parent), analysis); + return currentShape === "promise" + ? returnedOutcome(enclosingFunction(parent), analysis) + : returnedArrayOutcome(enclosingFunction(parent), analysis); case "ArrowFunctionExpression": - return get(parent, "body") === current - ? returnedFate(parent as FunctionLikeNode, analysis) - : "handled"; + if (get(parent, "body") !== current) return HANDLED; + return currentShape === "promise" + ? returnedOutcome(parent as FunctionLikeNode, analysis) + : returnedArrayOutcome(parent as FunctionLikeNode, analysis); case "ExpressionStatement": - return "dropped"; + return DROPPED; - // `void` is the way to say a promise is deliberately not awaited, and - // any other operator uses the value somewhere the rule cannot follow. + // `void` is the way to say a promise is deliberately not awaited. Any + // other operator (`!`, `typeof`, ...) makes no use of the promise. case "UnaryExpression": - return "handled"; + return get(parent, "operator") === "void" ? HANDLED : DROPPED; case "SequenceExpression": { const expressions = get(parent, "expressions") as Node[]; - if (expressions[expressions.length - 1] !== current) return "dropped"; + if (expressions[expressions.length - 1] !== current) return DROPPED; current = parent; continue; } case "ConditionalExpression": - if (get(parent, "test") === current) return "handled"; + if (get(parent, "test") === current) return HANDLED; current = parent; continue; case "LogicalExpression": - case "ArrayExpression": - case "SpreadElement": case "Property": case "ObjectExpression": current = parent; continue; + // A promise in an array literal is an array of promises. An array + // spread into one is flattened into it, and stays what it was. + case "ArrayExpression": + if (currentShape === "promises") return HANDLED; + currentShape = "promises"; + current = parent; + continue; + + case "SpreadElement": { + const array = parentOf(parent); + if (currentShape !== "promises" || array?.type !== "ArrayExpression") { + return HANDLED; + } + current = array; + continue; + } + case "MemberExpression": { - if (get(parent, "object") !== current) return "handled"; + if (get(parent, "object") !== current) return HANDLED; const call = parentOf(parent); const name = memberName(parent); if ( - call?.type === "CallExpression" && get(call, "callee") === parent && - name != null && PROMISE_CHAIN_METHODS.has(name) + currentShape === "promise" && call?.type === "CallExpression" && + get(call, "callee") === parent && name != null && + PROMISE_CHAIN_METHODS.has(name) ) { current = call; continue; } - return "handled"; + return HANDLED; } case "CallExpression": { - if (get(parent, "callee") === current) return "handled"; + if (get(parent, "callee") === current) return HANDLED; const callee = unwrap(asNode(get(parent, "callee")) ?? parent); const name = memberName(callee); - if (name === "waitUntil") return "handled"; + if (name === "waitUntil") return HANDLED; const object = callee.type === "MemberExpression" ? unwrap(asNode(get(callee, "object")) ?? callee) : null; @@ -318,9 +389,10 @@ function fateOf(expression: Node, analysis: Analysis): Fate { name != null && PROMISE_COMBINATORS.has(name) ) { current = parent; + currentShape = "promise"; continue; } - return "handled"; + return HANDLED; } // A promise kept in a variable is safe when the variable is used @@ -328,32 +400,35 @@ function fateOf(expression: Node, analysis: Analysis): Fate { case "VariableDeclarator": { const id = asNode(get(parent, "id")); if (get(parent, "init") !== current || id?.type !== "Identifier") { - return "handled"; + return HANDLED; } - return analysis.mentioned.has(id.name) ? "handled" : "dropped"; + return analysis.mentioned.has(id.name) ? HANDLED : DROPPED; } case "AssignmentExpression": { - if (get(parent, "right") !== current) return "handled"; + if (get(parent, "right") !== current) return HANDLED; const name = getAssignmentTargetName( asNode(get(parent, "left")) ?? parent, ); - if (name == null) return "handled"; - return analysis.mentioned.has(name) ? "handled" : "dropped"; + if (name == null) return HANDLED; + return analysis.mentioned.has(name) ? HANDLED : DROPPED; } default: - return "handled"; + return HANDLED; } } } -/** Reports every delivery call in `listener` whose promise is dropped. */ +/** Reports every place in `listener` where a delivery promise is dropped. */ function checkListener( listener: FunctionLikeNode, report: (node: Node) => void, ): void { - const targets = findDeliveryTargets(listener); + const scopes: UsedScope[] = []; + walkUsedScopes(listener.body as Node, (scope) => scopes.push(scope)); + + const targets = findDeliveryTargets(listener, scopes); if (targets.contextName == null && targets.aliases.size === 0) return; const isDeliveryCall = (call: CallExpression): boolean => { @@ -372,17 +447,19 @@ function checkListener( collectReferencedNames(listener.body, mentioned, true); const analysis: Analysis = { listener, mentioned }; - const scopes: UsedScope[] = []; - walkUsedScopes(listener.body as Node, (scope) => scopes.push(scope)); const callsByScope = scopes.map((scope) => { const calls: CallExpression[] = []; for (const statement of scope.statements) collectCalls(statement, calls); return calls; }); - // A local helper that delivers and hands its promise back, by awaiting the - // delivery or by returning it, is only as safe as the way it is called. - const carrying = new Set(); + // A function that delivers and carries the promise on, by awaiting the + // delivery or by returning it, is only as safe as what its own callers do + // with the promise it returns. `fateOf()` says which function the promise + // ended up in, which is not always the one that made the call: a delivery + // inside a `map()` callback lands in whatever function awaits or returns + // the `Promise.all()` around it. + const carrying = new Map(); const carriesDelivery = (call: CallExpression, scope: UsedScope): boolean => { if (isDeliveryCall(call)) return true; const callee = unwrap(call.callee as Node); @@ -390,30 +467,67 @@ function checkListener( const helpers = scope.functionsByName.get(callee.name); return helpers?.some((helper) => carrying.has(helper)) ?? false; }; + const noteCarrying = ({ fate, owner }: Outcome): boolean => { + if (owner == null || owner === listener) return false; + if (fate !== "awaited" && fate !== "returned") return false; + const known = carrying.get(owner); + if (known === "awaited" || known === fate) return false; + carrying.set(owner, fate); + return true; + }; for (let changed = true; changed;) { changed = false; scopes.forEach((scope, index) => { - const fn = scope.fn; - if (fn == null || carrying.has(fn)) return; for (const call of callsByScope[index]) { - if (!carriesDelivery(call, scope)) continue; - const fate = fateOf(call, analysis); - if (fate === "awaited" || fate === "returned") { - carrying.add(fn); + if ( + carriesDelivery(call, scope) && noteCarrying(fateOf(call, analysis)) + ) { changed = true; - return; } } }); } const reported = new Set(); + const flag = (node: Node): void => { + if (reported.has(node)) return; + reported.add(node); + report(node); + }; + + // A delivery call, or a call to a helper that delivers, whose promise is + // dropped. scopes.forEach((scope, index) => { for (const call of callsByScope[index]) { - if (reported.has(call) || !carriesDelivery(call, scope)) continue; - if (fateOf(call, analysis) !== "dropped") continue; - reported.add(call); - report(call); + if (!carriesDelivery(call, scope)) continue; + if (fateOf(call, analysis).fate === "dropped") flag(call); + } + }); + + // A callback that awaits a delivery hands its own promise to whoever runs + // it: `forEach()` drops it, an immediately invoked one is as safe as the + // call, and one given to `map()` is as safe as the array it builds. A + // callback that returns the delivery was already judged above. + for (const [fn, kind] of carrying) { + if ( + kind === "awaited" && returnedOutcome(fn, analysis).fate === "dropped" + ) { + flag(fn); + } + } + + // The same for a function that delivers when it is handed over by name, + // as in `inboxes.forEach(deliver)`. + scopes.forEach((scope, index) => { + for (const call of callsByScope[index]) { + for (const arg of call.arguments as Node[]) { + if (arg.type !== "Identifier") continue; + const held = scope.functionsByName.get(arg.name); + if (held == null || !held.some((fn) => carrying.has(fn))) continue; + if (callbackResultOutcome(call, analysis).fate === "dropped") { + flag(arg); + } + } } }); } diff --git a/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts b/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts index 8811325e0..7b981df57 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts @@ -1533,3 +1533,851 @@ federation expectedError: "Delivery is not awaited", }), ); + +test( + `${ruleName}: ❌ Bad - awaiting the array of promises that map returns`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - awaiting the array of promises that flatMap returns`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await inboxes.flatMap((target) => + ctx.sendActivity(sender, target, activity) + ); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - awaiting an array literal of promises`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await [ctx.sendActivity(sender, inbox, activity)]; + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - returning the array of promises that map returns from the listener`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + return inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - async callback passed to forEach`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + inboxes.forEach(async (target) => { + await ctx.sendActivity(sender, target, activity); + }); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - async callback passed to map, with the result dropped`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + inboxes.map(async (target) => { + await ctx.sendActivity(sender, target, activity); + }); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - async function invoked immediately without await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + (async () => { + await ctx.sendActivity(sender, inbox, activity); + })(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - named function that returns delivery, passed to forEach`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = (target) => ctx.sendActivity(sender, target, activity); + inboxes.forEach(deliver); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - named async function that awaits delivery, passed to forEach`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver(target) { + await ctx.sendActivity(sender, target, activity); + } + inboxes.forEach(deliver); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - named function that returns delivery, passed to map with the result dropped`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = (target) => ctx.sendActivity(sender, target, activity); + inboxes.map(deliver); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - named async function that awaits delivery, passed to a then that is not awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + } + Promise.resolve().then(deliver); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - helper returning Promise.all over a map, called without await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + function deliverAll() { + return Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + } + deliverAll(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - helper awaiting Promise.all over a map, called without await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliverAll() { + await Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + } + deliverAll(); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - not operator applied to a delivery promise`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + !ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - typeof applied to a delivery promise`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + typeof ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - Promise.race that is never awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.race([ctx.sendActivity(sender, inbox, activity)]); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - Promise.any that is never awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.any([ctx.sendActivity(sender, inbox, activity)]); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - helper that delivers but is only mentioned and never called`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => { + ctx.sendActivity(sender, inbox, activity); + }; + console.log(deliver); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ✅ Good - Promise.all over an async callback that awaits delivery`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.all( + inboxes.map(async (target) => { + await ctx.sendActivity(sender, target, activity); + }), + ); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - async function invoked immediately and awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await (async () => { + await ctx.sendActivity(sender, inbox, activity); + })(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - named async function passed to map inside Promise.all`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver(target) { + await ctx.sendActivity(sender, target, activity); + } + await Promise.all(inboxes.map(deliver)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - async callback handed to an unknown function`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + setTimeout(async () => { + await ctx.sendActivity(sender, inbox, activity); + }, 0); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited Promise.race`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.race([ctx.sendActivity(sender, inbox, activity)]); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited Promise.any`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await Promise.any([ctx.sendActivity(sender, inbox, activity)]); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper returning an array of promises, passed to Promise.all`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliverAll = () => inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + await Promise.all(deliverAll()); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper returning Promise.all, awaited by the caller`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliverAll() { + return Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + } + await deliverAll(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery method taken from ctx in a helper nothing uses`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + function unused() { + const send = ctx.sendActivity; + return send; + } + function send() {} + send(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery method taken from ctx in a dead branch`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + function send() {} + if (false) { + const send = ctx.sendActivity; + } + send(sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promise mentioned only in a dead branch`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const pending = ctx.sendActivity(sender, inbox, activity); + if (false) { + await pending; + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery promise handed to an unknown function`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + track(ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery promise used as a condition`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const label = ctx.sendActivity(sender, inbox, activity) ? "sent" : "not sent"; + console.log(label); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery promise as the last operand of an awaited sequence`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await (console.log("sending"), ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery promise handed to a constructor`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + new Tracker(ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - promise stored on an object that is read later`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const state = {}; + state.pending = ctx.sendActivity(sender, inbox, activity); + console.log(state); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited call through template literal bracket notation`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await ctx[\`sendActivity\`](sender, inbox, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - listener without a context parameter`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async () => { + console.log("no context to deliver with"); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - listener that destructures unrelated fields`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async ({ identifier, ...rest }, activity) => { + console.log(identifier, rest, activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery method with a default in the parameters, awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async ({ sendActivity = fallbackSend }, activity) => { + await sendActivity({ identifier: "alice" }, "followers", activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - bare call through template literal bracket notation`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx[\`sendActivity\`](sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - promise stored on an object that is never read`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const state = {}; + state.pending = ctx.sendActivity(sender, inbox, activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery method with a default in the parameters, called without await`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async ({ sendActivity = fallbackSend }, activity) => { + sendActivity({ identifier: "alice" }, "followers", activity); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); diff --git a/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts b/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts index 15588dd5d..51de78176 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts @@ -517,6 +517,324 @@ federation }; handlers.deliver(); }); +`, + ], + [ + "awaiting the array of promises that map returns", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + ], + [ + "awaiting the array of promises that flatMap returns", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await inboxes.flatMap((target) => + ctx.sendActivity(sender, target, activity) + ); + }); +`, + ], + [ + "awaiting an array literal of promises", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await [ctx.sendActivity(sender, inbox, activity)]; + }); +`, + ], + [ + "returning the array of promises that map returns from the listener", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + return inboxes.map((target) => ctx.sendActivity(sender, target, activity)); + }); +`, + ], + [ + "async callback passed to forEach", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + inboxes.forEach(async (target) => { + await ctx.sendActivity(sender, target, activity); + }); + }); +`, + ], + [ + "async callback passed to map, with the result dropped", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + inboxes.map(async (target) => { + await ctx.sendActivity(sender, target, activity); + }); + }); +`, + ], + [ + "async function invoked immediately without await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + (async () => { + await ctx.sendActivity(sender, inbox, activity); + })(); + }); +`, + ], + [ + "named function that returns delivery, passed to forEach", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = (target) => ctx.sendActivity(sender, target, activity); + inboxes.forEach(deliver); + }); +`, + ], + [ + "named async function that awaits delivery, passed to forEach", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver(target) { + await ctx.sendActivity(sender, target, activity); + } + inboxes.forEach(deliver); + }); +`, + ], + [ + "named function that returns delivery, passed to map with the result dropped", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = (target) => ctx.sendActivity(sender, target, activity); + inboxes.map(deliver); + }); +`, + ], + [ + "named async function that awaits delivery, passed to a then that is not awaited", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliver() { + await ctx.sendActivity(sender, inbox, activity); + } + Promise.resolve().then(deliver); + }); +`, + ], + [ + "helper returning Promise.all over a map, called without await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + function deliverAll() { + return Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + } + deliverAll(); + }); +`, + ], + [ + "helper awaiting Promise.all over a map, called without await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function deliverAll() { + await Promise.all(inboxes.map((target) => ctx.sendActivity(sender, target, activity))); + } + deliverAll(); + }); +`, + ], + [ + "not operator applied to a delivery promise", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + !ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "typeof applied to a delivery promise", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + typeof ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "Promise.race that is never awaited", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.race([ctx.sendActivity(sender, inbox, activity)]); + }); +`, + ], + [ + "Promise.any that is never awaited", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + Promise.any([ctx.sendActivity(sender, inbox, activity)]); + }); +`, + ], + [ + "helper that delivers but is only mentioned and never called", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => { + ctx.sendActivity(sender, inbox, activity); + }; + console.log(deliver); + }); +`, + ], + [ + "bare call through template literal bracket notation", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + ctx[\`sendActivity\`](sender, inbox, activity); + }); +`, + ], + [ + "promise stored on an object that is never read", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const state = {}; + state.pending = ctx.sendActivity(sender, inbox, activity); + }); +`, + ], + [ + "delivery method with a default in the parameters, called without await", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async ({ sendActivity = fallbackSend }, activity) => { + sendActivity({ identifier: "alice" }, "followers", activity); + }); `, ], ]; From ceffc5540da8239dd40e19168dca67328758d02e Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sat, 26 Sep 2026 15:33:32 +0900 Subject: [PATCH 08/11] Cover how listeners are found and how the reachability walk ends lib/outbox-listener.ts and lib/reachability.ts moved out of outbox-listener-delivery-required with several paths no test reached, which Codecov counted as new uncovered lines. Cover them through the existing rule: a listener held in an object property or behind an alias, one registered after authorize() and onError(), a call that is not a listener registration, and a listener that cannot be resolved; and for the walk, for-in and labeled loops, destructuring patterns that hold functions, and helpers that call each other. https://github.com/fedify-dev/fedify/pull/1067 Assisted-by: Claude Code:claude-sonnet-5 --- .../outbox-listener-delivery-required.test.ts | 155 ++++++++++++ .../tests/outbox-listener-discovery.test.ts | 232 ++++++++++++++++++ 2 files changed, 387 insertions(+) create mode 100644 packages/lint/src/tests/outbox-listener-discovery.test.ts diff --git a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts index 849479010..4b9526b76 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-required.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-required.test.ts @@ -2207,3 +2207,158 @@ federation ruleName, }), ); + +test( + `${ruleName}: ✅ Good - delivery inside a for-in loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + for (const key in { a: 1 }) { + await ctx.sendActivity(sender, inbox, activity); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery inside a labeled loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + outer: for (const target of [inbox]) { + await ctx.sendActivity(sender, target, activity); + break outer; + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper held in an array destructuring`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const [deliver] = [() => ctx.sendActivity(sender, inbox, activity)]; + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper held in a destructuring default`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const { deliver = () => ctx.sendActivity(sender, inbox, activity) } = {}; + await deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper held in a rest element`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const { ...handlers } = { deliver: () => ctx.sendActivity(sender, inbox, activity) }; + await handlers.deliver(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helpers that call each other and deliver`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function first() { + await second(); + } + async function second() { + await ctx.sendActivity(sender, inbox, activity); + await first(); + } + await first(); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad - helpers that call each other without delivering`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + async function first() { + await second(); + } + async function second() { + await first(); + } + await first(); + }); +`, + rule, + ruleName, + expectedError: "Outbox listeners should deliver posted activities", + }), +); diff --git a/packages/lint/src/tests/outbox-listener-discovery.test.ts b/packages/lint/src/tests/outbox-listener-discovery.test.ts new file mode 100644 index 000000000..5989fd988 --- /dev/null +++ b/packages/lint/src/tests/outbox-listener-discovery.test.ts @@ -0,0 +1,232 @@ +import { test } from "node:test"; +import { RULE_IDS } from "../lib/const.ts"; +import lintTest from "../lib/test.ts"; +import * as rule from "../rules/outbox-listener-delivery-required.ts"; + +// How a listener registration is found and resolved to a function, which +// the outbox listener rules share. + +const ruleName = RULE_IDS.outboxListenerDeliveryRequired; + +test( + `${ruleName}: ✅ Good listener held in an object literal property`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +const handlers = { + deliver: async (ctx, activity) => { + await ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity); + }, +}; +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, handlers.deliver); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad listener held in an object literal property that does not deliver`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +const handlers = { + deliver: async (ctx) => { + console.log(ctx.identifier); + }, +}; +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, handlers.deliver); +`, + rule, + ruleName, + expectedError: "Outbox listeners should deliver posted activities", + }), +); + +test( + `${ruleName}: ✅ Good listener passed through an alias of a named function`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +const handler = async (ctx, activity) => { + await ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity); +}; +const alias = handler; +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, alias); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad listener passed through an alias of a function that does not deliver`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +const handler = async (ctx) => { + console.log(ctx.identifier); +}; +const alias = handler; +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, alias); +`, + rule, + ruleName, + expectedError: "Outbox listeners should deliver posted activities", + }), +); + +test( + `${ruleName}: ✅ Good listener registered after authorize and onError`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .authorize(() => true) + .onError(() => {}) + .on(Activity, async (ctx, activity) => { + await ctx.sendActivity({ identifier: ctx.identifier }, "followers", activity); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ❌ Bad listener registered after authorize that does not deliver`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .authorize(() => true) + .on(Activity, async (ctx) => { + console.log(ctx.identifier); + }); +`, + rule, + ruleName, + expectedError: "Outbox listeners should deliver posted activities", + }), +); + +test( + `${ruleName}: ✅ Good on called on a plain object`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +const emitter = { on() {} }; + +emitter.on(Activity, async (ctx) => { + console.log(ctx.identifier); +}); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good on called on the result of an unrelated call`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +getEmitter().on(Activity, async (ctx) => { + console.log(ctx.identifier); +}); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good on called after an unrelated method in the chain`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .somethingElse() + .on(Activity, async (ctx) => { + console.log(ctx.identifier); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good listener that cannot be resolved`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, unknownHandler); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good listener bound to something that is not a function`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +const handler = 5; +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, handler); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good listener held in a computed property`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +const handlers = { + deliver: async (ctx) => { + console.log(ctx.identifier); + }, +}; +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, handlers["deliver"]); +`, + rule, + ruleName, + }), +); From ef5ffb6d5db942aa53c560566392dda5e2ac0cd9 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sat, 26 Sep 2026 15:33:42 +0900 Subject: [PATCH 09/11] Say what the delivery-not-awaited rule does with arrays and callbacks Promise.race() and Promise.any() do not wait for the delivery, they are accepted as a deliberate choice to stop waiting, like void, so the manual and the changelog no longer say they wait. Describe the array of promises that map() returns, async callbacks and the operators other than void, and note that the rule leans towards quiet where a name is only mentioned: a stored promise counts as used when its variable is mentioned anywhere, and a helper counts as running once its name is. Drop "opt-in" from the link in outbox-listener-delivery-required, since the ESLint recommended configuration enables the rule as a warning. https://github.com/fedify-dev/fedify/pull/1067#pullrequestreview-5323966182 Assisted-by: Claude Code:claude-sonnet-5 --- CHANGES.md | 4 ++- changes.d/lint/outbox-delivery-not-awaited.md | 4 ++- docs/manual/lint.md | 34 ++++++++++++++----- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 1f959d41d..22032ad9d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -289,7 +289,9 @@ To be released. activity may never leave on a runtime such as Cloudflare Workers, which discards pending work once the response is returned. A call counts as handled when its promise is awaited, returned, passed to `Promise.all()` - and its siblings, or handed to `waitUntil()`, and `void` opts a call out. + or `Promise.allSettled()`, or handed to `waitUntil()`, and `void`, + `Promise.race()` and `Promise.any()` are accepted as deliberate choices to + stop waiting. The ESLint `recommended` configuration enables the rule as a warning and `strict` as an error, and Oxlint users enable it by name. It is not available in Deno Lint, which turns on every rule of a plugin at once. diff --git a/changes.d/lint/outbox-delivery-not-awaited.md b/changes.d/lint/outbox-delivery-not-awaited.md index 79ff9f323..8a4a93287 100644 --- a/changes.d/lint/outbox-delivery-not-awaited.md +++ b/changes.d/lint/outbox-delivery-not-awaited.md @@ -9,7 +9,9 @@ links: activity may never leave on a runtime such as Cloudflare Workers, which discards pending work once the response is returned. A call counts as handled when its promise is awaited, returned, passed to `Promise.all()` - and its siblings, or handed to `waitUntil()`, and `void` opts a call out. + or `Promise.allSettled()`, or handed to `waitUntil()`, and `void`, + `Promise.race()` and `Promise.any()` are accepted as deliberate choices to + stop waiting. The ESLint `recommended` configuration enables the rule as a warning and `strict` as an error, and Oxlint users enable it by name. It is not available in Deno Lint, which turns on every rule of a plugin at once. diff --git a/docs/manual/lint.md b/docs/manual/lint.md index 66a242808..9668050b1 100644 --- a/docs/manual/lint.md +++ b/docs/manual/lint.md @@ -781,7 +781,7 @@ explicit delivery path, and that path must actually run. The rule checks that a delivery call exists and can run, not that the delivery completes, so a listener it accepts is not guaranteed to federate. A delivery -call that is never awaited is not reported here; the opt-in +call that is never awaited is not reported here; the [`outbox-listener-delivery-not-awaited`](#outbox-listener-delivery-not-awaited) rule checks for that. @@ -877,22 +877,40 @@ You've registered an outbox listener with `setOutboxListeners()`, and it calls `ctx.sendActivity()` or `ctx.forwardActivity()` in a way that drops the returned promise. The rule follows the promise from the call to where it ends up. A call counts as handled when its promise is awaited, returned, passed to -`Promise.all()`, `Promise.allSettled()`, `Promise.race()` or `Promise.any()`, -or handed to a method named `waitUntil()`. A call is reported when its promise -is discarded, including when it is only kept in a variable that nothing else -uses, or returned from a callback that is passed to `forEach()`. +`Promise.all()` or `Promise.allSettled()`, or handed to a method named +`waitUntil()`. A call is reported when its promise is discarded, including when +it is only kept in a variable that nothing else uses. When it cannot tell where a promise goes, the rule stays quiet. In practice: - - `void ctx.sendActivity(...)` is read as a deliberate choice and is not - reported. A discarded `.catch()`, `.then()` or `.finally()` chain is - reported, since a `.catch()` handles the error but does not wait. + - `void ctx.sendActivity(...)`, `Promise.race(...)` and `Promise.any(...)` are + read as deliberate choices to stop waiting, and are not reported. A + `race()` or `any()` whose own result is dropped is still reported, and so is + any other operator applied to a promise, such as `!` or `typeof`. A + discarded `.catch()`, `.then()` or `.finally()` chain is reported, since a + `.catch()` handles the error but does not wait. + - An array of promises, such as the result of `map()`, waits for nothing on + its own. Awaiting it, or returning it from the listener, is reported. It + counts as handled once it reaches `Promise.all()` or one of its siblings, + or when it is kept in a variable that is mentioned again. + - An `async` callback that awaits a delivery is judged by where the callback + goes, since its own promise is what carries the delivery. `forEach()` drops + that promise, so `inboxes.forEach(async (inbox) => { await ... })` is + reported. A callback that is invoked immediately, or given to `map()` or + `then()`, is only as safe as the result of that call. The same holds for a + function passed by name, as in `inboxes.forEach(deliver)`. - A local helper that delivers is judged by how it is called: `deliver();` is reported when `deliver()` awaits or returns a delivery, and `await deliver();` is not. - A promise passed to a function the rule does not know, such as `queue.push(...)` or `setTimeout(...)`, is left alone, since the rule cannot tell what that function does with it. + - The rule leans towards quiet where a name is only mentioned. A stored + promise counts as used when its variable is mentioned anywhere in the + listener, even only in a dead branch or in a helper that is never called. A + helper that delivers counts as running once its name is mentioned, even if + it is only stored or logged, so a bare delivery call inside it is still + reported. - As in `outbox-listener-delivery-required`, the rule reads only the listener body. A delivery call in a helper that is declared outside the listener, or in another module, is not seen. From bbc353f877c5f60090f5de4ff088c57e363fbde1 Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sun, 27 Sep 2026 08:32:24 +0900 Subject: [PATCH 10/11] Report objects and conditions that drop delivery The second review round found two more ways to leave a delivery in flight without the rule noticing. An object literal that holds a delivery promise, as in `await { pending: ctx.sendActivity(...) }`, was read as if it were the promise itself, but awaiting it waits for nothing inside it, the same as an array of promises. Follow the object as a shape of its own, so that awaiting it or returning it from the listener is reported. An object that a helper returns is left alone, like an array, since the helper's caller may read the promise back out of it. A promise used as the test of an if statement, a loop or a conditional expression was left alone as an unknown consumer, but a promise is always truthy, so testing one waits for nothing. Report it the way `!` and `typeof` already are. The test that pinned the old behavior for a conditional expression now expects the report. https://github.com/fedify-dev/fedify/pull/1067#pullrequestreview-5325544896 Assisted-by: Claude Code:claude-sonnet-5 --- .../outbox-listener-delivery-not-awaited.ts | 44 ++- ...tbox-listener-delivery-not-awaited.test.ts | 316 +++++++++++++++++- .../outbox-listener-delivery-rules.test.ts | 76 +++++ 3 files changed, 423 insertions(+), 13 deletions(-) diff --git a/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts b/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts index 6062b12a2..c72b26955 100644 --- a/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts +++ b/packages/lint/src/rules/outbox-listener-delivery-not-awaited.ts @@ -44,8 +44,10 @@ const DROPPED: Outcome = { fate: "dropped", owner: null }; * What the value being followed is. An array of promises, such as what * `map()` returns, waits for nothing until it reaches `Promise.all()` or one * of its siblings: awaiting or returning it leaves every promise in flight. + * So does an object with a promise in one of its properties, which nothing + * waits for at all. */ -type Shape = "promise" | "promises"; +type Shape = "promise" | "promises" | "object"; /** Wrappers that pass a value through unchanged. */ const TRANSPARENT_WRAPPERS = new Set([ @@ -272,11 +274,12 @@ function returnedOutcome( } /** - * Where an array of promises goes when it is returned. The listener's caller - * does not wait for what is inside it. Any other function hands it to its - * own callers, who may well pass it to `Promise.all()`. + * Where an array of promises, or an object holding one, goes when it is + * returned. The listener's caller does not wait for what is inside it. Any + * other function hands it to its own callers, who may well pass it to + * `Promise.all()` or read the promise back out of it. */ -function returnedArrayOutcome( +function returnedContainerOutcome( fn: FunctionLikeNode | null, analysis: Analysis, ): Outcome { @@ -301,7 +304,8 @@ function fateOf( } switch (parent.type) { - // Awaiting an array of promises waits for none of them. + // Awaiting an array of promises, or an object holding one, waits for + // none of them. case "AwaitExpression": return currentShape === "promise" ? { fate: "awaited", owner: enclosingFunction(parent) } @@ -310,13 +314,13 @@ function fateOf( case "ReturnStatement": return currentShape === "promise" ? returnedOutcome(enclosingFunction(parent), analysis) - : returnedArrayOutcome(enclosingFunction(parent), analysis); + : returnedContainerOutcome(enclosingFunction(parent), analysis); case "ArrowFunctionExpression": if (get(parent, "body") !== current) return HANDLED; return currentShape === "promise" ? returnedOutcome(parent as FunctionLikeNode, analysis) - : returnedArrayOutcome(parent as FunctionLikeNode, analysis); + : returnedContainerOutcome(parent as FunctionLikeNode, analysis); case "ExpressionStatement": return DROPPED; @@ -333,25 +337,41 @@ function fateOf( continue; } + // A promise is always truthy, so testing one waits for nothing. A `for` + // loop discards the value of its initializer and its update as well. case "ConditionalExpression": - if (get(parent, "test") === current) return HANDLED; + if (get(parent, "test") === current) return DROPPED; current = parent; continue; + case "IfStatement": + case "WhileStatement": + case "DoWhileStatement": + case "ForStatement": + return DROPPED; + case "LogicalExpression": case "Property": + current = parent; + continue; + + // A promise in an object literal is an object holding a promise. An + // array of promises in one is still an array of promises. case "ObjectExpression": + if (currentShape === "promise") currentShape = "object"; current = parent; continue; - // A promise in an array literal is an array of promises. An array - // spread into one is flattened into it, and stays what it was. + // A promise in an array literal is an array of promises. An array or an + // object in one is left alone. case "ArrayExpression": - if (currentShape === "promises") return HANDLED; + if (currentShape !== "promise") return HANDLED; currentShape = "promises"; current = parent; continue; + // An array spread into an array literal is flattened into it, and stays + // an array of promises. case "SpreadElement": { const array = parentOf(parent); if (currentShape !== "promises" || array?.type !== "ArrayExpression") { diff --git a/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts b/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts index 7b981df57..24f6a37f8 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-not-awaited.test.ts @@ -2175,7 +2175,7 @@ federation ); test( - `${ruleName}: ✅ Good - delivery promise used as a condition`, + `${ruleName}: ❌ Bad - delivery promise used as the test of a conditional expression`, lintTest({ code: ` import { Activity } from "@fedify/vocab"; @@ -2191,6 +2191,7 @@ federation `, rule, ruleName, + expectedError: "Delivery is not awaited", }), ); @@ -2381,3 +2382,316 @@ federation expectedError: "Delivery is not awaited", }), ); + +test( + `${ruleName}: ❌ Bad - awaiting an object literal that holds a delivery promise`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await { pending: ctx.sendActivity(sender, inbox, activity) }; + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - returning an object literal that holds a delivery promise from the listener`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + return { pending: ctx.sendActivity(sender, inbox, activity) }; + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - awaiting an object literal that holds a delivery promise deeper down`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await { outer: { pending: ctx.sendActivity(sender, inbox, activity) } }; + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - awaiting an object literal that holds an array of delivery promises`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await { all: inboxes.map((target) => ctx.sendActivity(sender, target, activity)) }; + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery promise used as the test of an if statement`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (ctx.sendActivity(sender, inbox, activity)) { + console.log("sent"); + } + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery promise used as the test of a while loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + while (ctx.sendActivity(sender, inbox, activity)) { + break; + } + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery promise used as the test of a do-while loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + do { + console.log("once"); + } while (ctx.sendActivity(sender, inbox, activity)); + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery promise used as the test of a for loop`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + for (; ctx.sendActivity(sender, inbox, activity);) { + break; + } + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - delivery promise as an operand in the test of an if statement`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (activity != null && ctx.sendActivity(sender, inbox, activity)) { + console.log("sent"); + } + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ❌ Bad - helper that delivers, called as the test of an if statement`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => ctx.sendActivity(sender, inbox, activity); + if (deliver()) { + console.log("sent"); + } + }); +`, + rule, + ruleName, + expectedError: "Delivery is not awaited", + }), +); + +test( + `${ruleName}: ✅ Good - awaited delivery used as the test of an if statement`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (await ctx.sendActivity(sender, inbox, activity)) { + console.log("sent"); + } + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - awaited delivery in a branch of a conditional expression`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await (activity.id != null ? ctx.sendActivity(sender, inbox, activity) : null); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - helper returning an object that holds a delivery promise`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => ({ pending: ctx.sendActivity(sender, inbox, activity) }); + const { pending } = deliver(); + await pending; + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - object holding a delivery promise handed to an unknown function`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + queue.push({ pending: ctx.sendActivity(sender, inbox, activity) }); + }); +`, + rule, + ruleName, + }), +); + +test( + `${ruleName}: ✅ Good - delivery promise taken out of an object literal and awaited`, + lintTest({ + code: ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const { pending } = { pending: ctx.sendActivity(sender, inbox, activity) }; + await pending; + }); +`, + rule, + ruleName, + }), +); diff --git a/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts b/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts index 51de78176..b166415cc 100644 --- a/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts +++ b/packages/lint/src/tests/outbox-listener-delivery-rules.test.ts @@ -835,6 +835,82 @@ federation .on(Activity, async ({ sendActivity = fallbackSend }, activity) => { sendActivity({ identifier: "alice" }, "followers", activity); }); +`, + ], + [ + "awaited object literal that holds a delivery promise", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + await { pending: ctx.sendActivity(sender, inbox, activity) }; + }); +`, + ], + [ + "object literal that holds a delivery promise, returned from the listener", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + return { pending: ctx.sendActivity(sender, inbox, activity) }; + }); +`, + ], + [ + "delivery promise used as the test of a conditional expression", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const label = ctx.sendActivity(sender, inbox, activity) ? "sent" : "not sent"; + console.log(label); + }); +`, + ], + [ + "delivery promise used as the test of an if statement", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + if (ctx.sendActivity(sender, inbox, activity)) { + console.log("sent"); + } + }); +`, + ], + [ + "helper that delivers, called as the test of an if statement", + ` +import { Activity } from "@fedify/vocab"; + +federation + .setOutboxListeners("/users/{identifier}/outbox") + .on(Activity, async (ctx, activity) => { + const sender = { identifier: ctx.identifier }; + const inbox = new URL("https://example.com/inbox"); + const deliver = () => ctx.sendActivity(sender, inbox, activity); + if (deliver()) { + console.log("sent"); + } + }); `, ], ]; From 3d5b6322d26711d751efa80249e653ec5a47319e Mon Sep 17 00:00:00 2001 From: Jae-Hyuk-Jang Date: Sun, 27 Sep 2026 08:32:36 +0900 Subject: [PATCH 11/11] Document objects, conditions and helper arrays List what the delivery-not-awaited rule does with an object literal that holds a promise and with a promise used as a test, and add the gap the review asked to have written down: an array of promises, or an object holding one, that a local helper returns is not followed to where the helper is called, so `await deliverAll()` and a bare `deliverAll()` are not reported. Following it is tracked in #1070. https://github.com/fedify-dev/fedify/pull/1067#pullrequestreview-5325544896 https://github.com/fedify-dev/fedify/issues/1070 Assisted-by: Claude Code:claude-sonnet-5 --- docs/manual/lint.md | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/docs/manual/lint.md b/docs/manual/lint.md index 9668050b1..f0537b6c7 100644 --- a/docs/manual/lint.md +++ b/docs/manual/lint.md @@ -886,13 +886,17 @@ When it cannot tell where a promise goes, the rule stays quiet. In practice: - `void ctx.sendActivity(...)`, `Promise.race(...)` and `Promise.any(...)` are read as deliberate choices to stop waiting, and are not reported. A `race()` or `any()` whose own result is dropped is still reported, and so is - any other operator applied to a promise, such as `!` or `typeof`. A - discarded `.catch()`, `.then()` or `.finally()` chain is reported, since a - `.catch()` handles the error but does not wait. + any other operator applied to a promise, such as `!` or `typeof`. So is a + promise used as the test of an `if` statement, a loop or a conditional + expression, since a promise is always truthy and testing it waits for + nothing. A discarded `.catch()`, `.then()` or `.finally()` chain is + reported, since a `.catch()` handles the error but does not wait. - An array of promises, such as the result of `map()`, waits for nothing on - its own. Awaiting it, or returning it from the listener, is reported. It - counts as handled once it reaches `Promise.all()` or one of its siblings, - or when it is kept in a variable that is mentioned again. + its own, and neither does an object that holds a promise, such as + `{ pending: ctx.sendActivity(...) }`. Awaiting one, or returning it from + the listener, is reported. An array counts as handled once it reaches + `Promise.all()` or one of its siblings, and either one counts as handled + when it is kept in a variable that is mentioned again. - An `async` callback that awaits a delivery is judged by where the callback goes, since its own promise is what carries the delivery. `forEach()` drops that promise, so `inboxes.forEach(async (inbox) => { await ... })` is @@ -901,7 +905,11 @@ When it cannot tell where a promise goes, the rule stays quiet. In practice: function passed by name, as in `inboxes.forEach(deliver)`. - A local helper that delivers is judged by how it is called: `deliver();` is reported when `deliver()` awaits or returns a delivery, and - `await deliver();` is not. + `await deliver();` is not. An array of promises, or an object holding one, + that a helper returns is not followed to where the helper is called, since + the caller may pass it to `Promise.all()`. So `await deliverAll();` and a + bare `deliverAll();` are not reported when `deliverAll()` returns the + result of `map()`. - A promise passed to a function the rule does not know, such as `queue.push(...)` or `setTimeout(...)`, is left alone, since the rule cannot tell what that function does with it.