diff --git a/packages/sdk/package-lock.json b/packages/sdk/package-lock.json index b0b5f65e3..267978c41 100644 --- a/packages/sdk/package-lock.json +++ b/packages/sdk/package-lock.json @@ -14,6 +14,7 @@ "@relayfile/relay-helpers": "0.4.12", "@relayflows/surface": "2.0.30", "@types/js-yaml": "^4.0.9", + "acorn": "^8.18.0", "ai-hist": "0.4.1", "ajv": "^8.17.1", "ajv-draft-04": "^1.0.0", @@ -1316,6 +1317,18 @@ "node": ">= 0.6" } }, + "node_modules/acorn": { + "version": "8.18.0", + "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.18.0.tgz", + "integrity": "sha512-lGq+9yr1/GuAWaVYIHRjvvySG5/4VfKIvC8EWxStPdcDh/Ka7FG3twP6v4d5BkravUilhIAsG4Qj83t02LWUPQ==", + "license": "MIT", + "bin": { + "acorn": "bin/acorn" + }, + "engines": { + "node": ">=0.4.0" + } + }, "node_modules/ai-hist": { "version": "0.4.1", "resolved": "https://registry.npmjs.org/ai-hist/-/ai-hist-0.4.1.tgz", diff --git a/packages/sdk/package.json b/packages/sdk/package.json index 1db90ae23..cb43d84df 100644 --- a/packages/sdk/package.json +++ b/packages/sdk/package.json @@ -51,6 +51,7 @@ "@relayfile/relay-helpers": "0.4.12", "@relayflows/surface": "2.0.30", "@types/js-yaml": "^4.0.9", + "acorn": "^8.18.0", "ai-hist": "0.4.1", "ajv": "^8.17.1", "ajv-draft-04": "^1.0.0", @@ -80,10 +81,5 @@ "@agent-relay/sdk": { "optional": true } - }, - "overrides": { - "@relayflows/surface": { - "@relayfile/relay-helpers": "$@relayfile/relay-helpers" - } } } diff --git a/packages/sdk/src/flow-requirements.ts b/packages/sdk/src/flow-requirements.ts index 7c589e590..1f381a62e 100644 --- a/packages/sdk/src/flow-requirements.ts +++ b/packages/sdk/src/flow-requirements.ts @@ -4,6 +4,7 @@ import type { TriggerSource } from '@relayflows/surface'; import { providerDeclaration } from './provider-trigger-contract.js'; import type { FlowSpec } from './spec.js'; import { helperCall } from './yaml-helpers.js'; +import { helperNamespacesUsed } from './helper-reference.js'; /** * What a flow needs from the workspace it deploys into, read from inert @@ -155,10 +156,14 @@ export function flowRequirements( // payload as input, and handler bodies are not dispatched yet (flows #301). // A handler's *trigger* is still a requirement — it is what wakes the flow. const text = typeof flow.body === 'function' ? Function.prototype.toString.call(flow.body) : ''; - const root = contextParameter(text); - if (root !== undefined) { + const rootName = contextParameter(text); + const root = rootName === undefined ? undefined : escapeRegExp(rootName); + if (root !== undefined && rootName !== undefined) { + // The AST walk compares against an Identifier name, so it needs the raw + // parameter; the regex scanners below need the escaped one. + const referenced = helperNamespacesUsed(text, rootName); for (const { provider, namespace } of helperProviders) { - if (helperReference(root, namespace).test(text)) declare({ provider, from: 'helper', detail: `f.${namespace}` }); + if (referenced.has(namespace)) declare({ provider, from: 'helper', detail: `f.${namespace}` }); } for (const use of workerCalls(root, text)) need(use.cli === undefined ? fallback : harnessFromCli(use.cli), use.detail); // `f.human(q, { to: "slack:#eng" })` is delivered by Cloud through that @@ -219,15 +224,25 @@ function stringList(value: unknown): string[] { return Array.isArray(value) ? value.filter((entry): entry is string => typeof entry === 'string') : []; } -/** The body's first parameter (`f` in `async (f, input) => …`), escaped for a pattern. */ +/** + * The body's first parameter (`f` in `async (f, input) => …`), RAW. + * + * Returned unescaped because the AST walk compares it to an `Identifier` + * name: escaping turned a legal parameter like `f$` into `f\$`, which matches + * no identifier and hid every helper call in that flow. Callers that build a + * pattern escape it themselves with `escapeRegExp`. + */ function contextParameter(body: string): string | undefined { - const parameter = body.match(/^(?:async\s+)?(?:function(?:\s+[\w$]+)?\s*)?(?:\(\s*([\w$]+)|([\w$]+)\s*=>)/u); - return (parameter?.[1] ?? parameter?.[2])?.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&'); + // The third alternative is an object-literal method (`async post(f) { … }`), + // a shape `Function.prototype.toString()` can return and which the first two + // do not match — leaving `root` undefined and the body unscanned entirely. + const parameter = body.match(/^(?:async\s+)?(?:function\s*\*?\s*(?:[\w$]+)?\s*)?(?:\(\s*([\w$]+)|([\w$]+)\s*=>|\*?\s*[\w$]+\s*\(\s*([\w$]+))/u); + return parameter?.[1] ?? parameter?.[2] ?? parameter?.[3]; } -/** Same recognition as `preflightHelpers`: `f.slack`, `f .slack`, `f["slack"]`. */ -function helperReference(root: string, namespace: string): RegExp { - return new RegExp(`(?:^|[^\\w$.])${root}\\s*(?:\\.\\s*${namespace}\\b|\\[\\s*['"]${namespace}['"]\\s*\\])`, 'u'); +/** Escape a literal for embedding in a RegExp source. */ +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&'); } /** diff --git a/packages/sdk/src/helper-preflight.ts b/packages/sdk/src/helper-preflight.ts index 3f22d264a..112192c2a 100644 --- a/packages/sdk/src/helper-preflight.ts +++ b/packages/sdk/src/helper-preflight.ts @@ -1,5 +1,6 @@ import { helperProviders } from '@relayflows/surface/runtime'; import type { PreflightResult, PreflightDiagnostic } from './preflight.js'; +import { helperNamespacesUsed } from './helper-reference.js'; /** Static discovery never executes the body; dynamic aliases are checked at call time. */ export function preflightHelpers( @@ -8,12 +9,18 @@ export function preflightHelpers( providers?: Readonly> }, ): PreflightResult { const body = typeof definition.body === 'function' ? Function.prototype.toString.call(definition.body) : ''; - const parameter = body.match(/^(?:async\s+)?(?:function(?:\s+[\w$]+)?\s*)?(?:\(\s*([\w$]+)|([\w$]+)\s*=>)/); - const root = (parameter?.[1] ?? parameter?.[2])?.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const parameter = body.match(/^(?:async\s+)?(?:function\s*\*?\s*(?:[\w$]+)?\s*)?(?:\(\s*([\w$]+)|([\w$]+)\s*=>|\*?\s*[\w$]+\s*\(\s*([\w$]+))/u); + // NOT regex-escaped: this is compared to an AST Identifier name, so a legal + // parameter like `f$` must stay `f$`. Escaping it hid every helper call. + const root = parameter?.[1] ?? parameter?.[2] ?? parameter?.[3]; const diagnostics: PreflightDiagnostic[] = []; + // Read from a parse, not from the text: a helper named inside a string, + // comment, template quasi or regex is not used, and refusing on one demands + // a mount the flow never touches. + const referenced = root === undefined ? new Set() : helperNamespacesUsed(body, root); for (const { provider, namespace, supported } of helperProviders) { const used = definition.header?.tools?.[namespace] === true - || (root !== undefined && new RegExp(`(?:^|[^\\w$.])${root}\\s*(?:\\.\\s*${namespace}\\b|\\[\\s*['"]${namespace}['"]\\s*\\])`).test(body)); + || referenced.has(namespace); if (!used) continue; const fact = facts.providers?.[provider] ?? (provider === 'slack' ? { mount: facts.slackMount, mock: facts.slackMock, token: facts.slackToken } diff --git a/packages/sdk/src/helper-reference.ts b/packages/sdk/src/helper-reference.ts new file mode 100644 index 000000000..fa7d1bf2e --- /dev/null +++ b/packages/sdk/src/helper-reference.ts @@ -0,0 +1,130 @@ +import { parse, parseExpressionAt } from 'acorn'; + +/** + * Which `f.` helpers a flow body actually USES. + * + * Read from a parse, not from the text. A helper merely NAMED in a string, a + * comment, a template quasi or a regex is not used, and declaring a + * requirement from one refuses a flow for a mount it never touches: + * `f.run("echo f.gitlab")`, an agent prompt naming `f.slack`, or a PR title in + * a commit message were each enough to demand a GitLab mount. + * + * This was first written as a lexer that blanked comments, strings and + * regexes. That is the wrong tool: `/` is a regex or a division depending on + * whether the preceding token ends an expression, which is not decidable + * without parse context, and the two shapes that ambiguity broke — + * interpolations (`${await f.gitlab…}`) and a quote inside a regex (`/'/`) — + * both HID real helper use, deploying a flow without its mount to fail at + * runtime. A parser settles every one of those by construction. + * + * What a parser still cannot see is indirection: `const p = 'gitlab'; + * f[p].issues…` is invisible to any static reading, because it needs value + * tracking. This scan is therefore a convenience for the literal case, never + * the authority — `header.tools[namespace] === true` is the declaration that + * is, and it short-circuits this entirely. + */ +export function helperNamespacesUsed(body: string, root: string): ReadonlySet { + const program = parseFlowBody(body); + if (program === null) return textFallback(body, root); + const used = new Set(); + walk(program, (node) => { + if (node.type !== 'MemberExpression') return; + const object = node.object as AstNode | undefined; + if (object?.type !== 'Identifier' || object.name !== root) return; + const namespace = memberName(node); + if (namespace !== undefined) used.add(namespace); + }); + return used; +} + +/** `f.slack`, `f["slack"]`, `f?.slack` — but not `f[variable]`, which is unknowable. */ +function memberName(node: AstNode): string | undefined { + const property = node.property as AstNode | undefined; + if (property === undefined) return undefined; + if (node.computed !== true) return property.type === 'Identifier' ? property.name : undefined; + return property.type === 'Literal' && typeof property.value === 'string' ? property.value : undefined; +} + +/** + * Parse whatever `Function.prototype.toString()` produced. + * + * It can be an arrow, a function expression, an async method shorthand from an + * object literal, or a declaration — only some of which are expressions, so + * each shape gets a try. Types are already stripped by the time a body reaches + * here (Node strips before evaluating, and bundlers transpile), so this is + * plain JavaScript. + */ +function parseFlowBody(body: string): AstNode | null { + const options = { ecmaVersion: 'latest' as const, allowAwaitOutsideFunction: true, allowReturnOutsideFunction: true }; + // `parseExpressionAt` stops at the end of the first expression and does not + // object to what follows, so `async post(f) { … }` parses as the identifier + // `async` and reports success having read three characters. Every attempt + // must therefore consume the whole source, or a method body would be walked + // as its own name and declare no helpers at all. + const whole = (source: string, node: AstNode): AstNode => { + const end = typeof node.end === 'number' ? node.end : -1; + if (end < 0 || source.slice(end).trim() !== '') throw new SyntaxError('unconsumed input'); + return node; + }; + for (const attempt of [ + () => whole(body, parseExpressionAt(body, 0, options) as unknown as AstNode), + () => whole(`(${body})`, parseExpressionAt(`(${body})`, 0, options) as unknown as AstNode), + () => parse(body, options) as unknown as AstNode, + // An object-literal method (`async post(f) { … }`) is neither expression + // nor statement on its own; it only parses inside an object. + () => whole(`({${body}})`, parseExpressionAt(`({${body}})`, 0, options) as unknown as AstNode), + ]) { + try { + return attempt(); + } catch { + continue; + } + } + return null; +} + +/** + * Last resort when nothing parses: match the text. + * + * Deliberately the permissive direction. An unparseable body is a shape this + * code does not understand, and under-reporting would deploy a flow without a + * mount it needs and fail at the call; over-reporting only asks for a mount + * that may go unused. Reaching here at all is a bug worth hearing about. + */ +function textFallback(body: string, root: string): ReadonlySet { + const used = new Set(); + const escaped = root.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&'); + const pattern = new RegExp(`(?:^|[^\\w$.])${escaped}\\s*(?:\\.\\s*([\\w$]+)|\\[\\s*['"]([^'"]+)['"]\\s*\\])`, 'gu'); + for (const match of body.matchAll(pattern)) { + const namespace = match[1] ?? match[2]; + if (namespace !== undefined) used.add(namespace); + } + return used; +} + +type AstNode = { + type: string; + end?: number; + name?: string; + value?: unknown; + computed?: boolean; + [key: string]: unknown; +}; + +/** Depth-first over every child node, without pulling in a second package. */ +function walk(node: AstNode, visit: (node: AstNode) => void): void { + visit(node); + for (const key of Object.keys(node)) { + if (key === 'type' || key === 'start' || key === 'end' || key === 'loc') continue; + const child = node[key]; + if (Array.isArray(child)) { + for (const entry of child) if (isNode(entry)) walk(entry, visit); + } else if (isNode(child)) { + walk(child, visit); + } + } +} + +function isNode(value: unknown): value is AstNode { + return typeof value === 'object' && value !== null && typeof (value as { type?: unknown }).type === 'string'; +} diff --git a/packages/sdk/tests/helper-reference.test.ts b/packages/sdk/tests/helper-reference.test.ts new file mode 100644 index 000000000..bfcfcade8 --- /dev/null +++ b/packages/sdk/tests/helper-reference.test.ts @@ -0,0 +1,164 @@ +import { describe, expect, it } from 'vitest'; +import { preflightHelpers } from '../src/preflight.js'; +import { flowRequirements } from '../src/flow-requirements.js'; +import { flow } from '@relayflows/surface'; +import { getFlowDefinition } from '@relayflows/surface/runtime'; + +/** + * A helper NAMED in a string or a comment is not a helper USED. Reading the + * body as text refused flows for mounts they never touch — a shell command + * echoing `f.gitlab`, an agent prompt naming `f.slack`, a PR title in a commit + * message. Both readers (preflight and requirements) must read it as syntax. + */ +const mountFacts = { providers: {} as Record }; +const refusals = (body: Function): string[] => + preflightHelpers({ header: {}, body }, mountFacts).diagnostics + .filter((d) => d.severity === 'refusal') + .map((d) => d.kind); + +describe('helper references are read as syntax, not text', () => { + it('does not demand a mount for a helper named inside a string', () => { + expect(refusals(async (f: any) => { await f.run('echo f.gitlab'); })).toEqual([]); + expect(refusals(async (f: any) => { await f.run('echo f.github'); })).toEqual([]); + }); + + it('does not demand a mount for a helper named in an agent prompt', () => { + expect(refusals(async (f: any) => { + await f.agent('a', { cli: 'claude', task: 'explain how f.slack.post works' }); + })).toEqual([]); + }); + + it('does not demand a mount for a helper named in a comment', () => { + expect(refusals(async (f: any) => { + // f.gitlab.issues.list is deliberately only mentioned here + await f.run('true'); + })).toEqual([]); + }); + + it('still demands a mount for real dot access', () => { + expect(refusals(async (f: any) => { await f.gitlab.issues.list({}); })) + .toContain('helper_provider.mount_required'); + }); + + it('still demands a mount for real bracket access', () => { + expect(refusals(async (f: any) => { await f['gitlab'].issues.list({}); })) + .toContain('helper_provider.mount_required'); + }); + + // A template quasi is text; a `${…}` is live code. Blanking the whole + // template hid real helper calls — a flow deploying without its mount and + // failing at runtime, which is worse than the false refusal this fixes. + it('sees a helper called from a template interpolation', () => { + expect(refusals(async (f: any) => { await f.run(`echo ${await f.gitlab.issues.list({})}`); })) + .toContain('helper_provider.mount_required'); + }); + + it('sees a helper called from a nested template interpolation', () => { + expect(refusals(async (f: any) => { await f.run(`a ${`b ${await f.gitlab.issues.list({})}`}`); })) + .toContain('helper_provider.mount_required'); + }); + + it('does not demand a mount for a helper named in template TEXT', () => { + expect(refusals(async (f: any) => { await f.run(`echo f.gitlab now`); })).toEqual([]); + }); + + // A regex is not code: `/f.gitlab/` is a mention, and a quote inside one + // would otherwise open a phantom string and blank the rest of the body. + it('does not read a helper named inside a regex literal as use', () => { + expect(refusals(async (f: any) => { const p = /f.gitlab/; await f.run('true'); return p; })).toEqual([]); + }); + + it('still sees a real helper call after a regex containing a quote', () => { + expect(refusals(async (f: any) => { const a = /'/; await f.gitlab.issues.list({}); return a; })) + .toContain('helper_provider.mount_required'); + }); + + it('does not mistake division for a regex', () => { + expect(refusals(async (f: any) => { const n = 10 / 2; await f.run('true'); return n; })).toEqual([]); + }); + + // Shapes a lexer cannot settle. `/` is a regex or a division depending on + // whether the previous token ends an expression, which needs parse context; + // and the blanking version missed optional chaining outright, passing a + // flow that really used the helper. + it('sees a helper reached through optional chaining', () => { + expect(refusals(async (f: any) => { await f?.gitlab?.issues.list({}); })) + .toContain('helper_provider.mount_required'); + }); + + it('sees a helper used inside a nested arrow', () => { + expect(refusals(async (f: any) => { + await Promise.all([1].map(async () => f.gitlab.issues.list({}))); + })).toContain('helper_provider.mount_required'); + }); + + it('reads a regex after a closing paren as a regex, not division', () => { + expect(refusals(async (f: any) => { + if (String(1).match(/f.gitlab/)) { await f.run('x'); } + })).toEqual([]); + }); + + it('reads division after a closing paren as division', () => { + expect(refusals(async (f: any) => { + const n = (1 + 2) / 2; await f.run('echo f.gitlab'); return n; + })).toEqual([]); + }); + + it('does not treat a matching object key as helper use', () => { + expect(refusals(async (f: any) => { const o = { gitlab: 1 }; await f.run('true'); return o; })).toEqual([]); + }); + + // The context parameter is compared to an AST Identifier name, so escaping + // it for a regex (as the pre-parser code did) hid every call in the flow. + it('sees helpers when the context parameter contains a regex metacharacter', () => { + expect(refusals(async (f$: any) => { await f$.gitlab.issues.list({}); })) + .toContain('helper_provider.mount_required'); + }); + + // `parseExpressionAt` stops at the first expression without objecting to the + // rest, so a method body parsed as the identifier `async` and declared + // nothing. Every attempt must consume the whole source. + it('sees helpers in an object-literal method body', () => { + const holder = { async post(f: any) { await f.gitlab.issues.list({}); } }; + expect(refusals(holder.post)).toContain('helper_provider.mount_required'); + }); + + it('sees helpers in non-async and generator method bodies', () => { + const holder = { + plain(f: any) { return f.gitlab.issues.list({}); }, + *gen(f: any) { return f.gitlab.issues.list({}); }, + }; + expect(refusals(holder.plain)).toContain('helper_provider.mount_required'); + expect(refusals(holder.gen as never)).toContain('helper_provider.mount_required'); + }); + + // `flowRequirements` has its OWN parameter extraction, mirroring + // `preflightHelpers`. The shapes above are asserted through preflight, so + // without these the mirrored regex could be reverted with the suite green. + it('declares helpers for every body shape through flowRequirements too', () => { + const providersFor = (body: unknown): string[] => + flowRequirements({ body } as never).integrations.map((i) => i.provider); + const holder = { + async asyncMethod(f: any) { return f.gitlab.issues.list({}); }, + plain(f: any) { return f.gitlab.issues.list({}); }, + *gen(f: any) { return f.gitlab.issues.list({}); }, + }; + expect(providersFor(holder.asyncMethod)).toContain('gitlab'); + expect(providersFor(holder.plain)).toContain('gitlab'); + expect(providersFor(holder.gen)).toContain('gitlab'); + expect(providersFor(async (f$: any) => { await f$.gitlab.issues.list({}); })).toContain('gitlab'); + expect(providersFor(async (f: any) => { await f.run('echo f.gitlab'); })).not.toContain('gitlab'); + }); + + it('does not declare a requirement from a mentioned helper', () => { + const mentioned = getFlowDefinition(flow('mention', async (ctx: any) => { + await ctx.run('echo f.gitlab'); + })); + expect(flowRequirements(mentioned).integrations.map((i) => i.provider)).not.toContain('gitlab'); + + const used = getFlowDefinition(flow('use', async (ctx: any) => { + await ctx.gitlab.issues.list({}); + })); + expect(flowRequirements(used).integrations.map((i) => i.provider)).toContain('gitlab'); + }); +});