From 5357164400ab0b576c1c1d3da375c3e89815a2a5 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Wed, 23 Sep 2026 22:21:05 -0700 Subject: [PATCH 1/6] fix(sdk): read helper references as syntax, and drop the surface peer override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cleanups that both became possible once surface 2.0.30 published. 1. A helper NAMED in a string or comment is not a helper USED. Both readers tested the raw body text, so `f.run("echo f.gitlab")`, an agent prompt naming `f.slack`, or a PR title in a commit message each declared a helper requirement and refused the flow with `helper_provider.mount_required` for a mount it never touches. That also made the SDK stricter than Cloud, whose `flow-source-requirements.ts` already reads these shapes as syntax — a flow Cloud accepts could be refused locally. Dot access is now read on the copy with comments AND strings blanked; bracket access (`f["gitlab"]`, whose key IS a string) on the comments-only copy, confirmed against the fully blanked one so the `f[` must have survived. Extracted to helper-reference.ts because both `preflightHelpers` and `flowRequirements` carried their own copy of the regex — the comment in flow-requirements.ts said "same recognition as preflightHelpers", which is how the two drifted into the same bug twice. 2. Removes the `overrides` entry added in #537. It existed only because the then-published surface pinned an exact peer `relay-helpers 0.4.11` against this repo's 0.4.12; surface 2.0.30 ships peer 0.4.12, so it is now inert. Verified, not assumed: `npm install` and a clean `npm ci` both exit 0 without it, with the @relayfile/sdk peer still installed. Regression test asserts both directions and is red against main's code (2 failed) and green here (6 passed). Surface gate exits 0. The daemon-backed suites fail identically before and after (7/7 on tests/yaml-local-agent-live either way) — they need a live relayflowd this environment lacks. Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk/package.json | 5 - packages/sdk/src/flow-requirements.ts | 9 +- packages/sdk/src/helper-preflight.ts | 6 +- packages/sdk/src/helper-reference.ts | 104 ++++++++++++++++++++ packages/sdk/tests/helper-reference.test.ts | 59 +++++++++++ 5 files changed, 171 insertions(+), 12 deletions(-) create mode 100644 packages/sdk/src/helper-reference.ts create mode 100644 packages/sdk/tests/helper-reference.test.ts diff --git a/packages/sdk/package.json b/packages/sdk/package.json index 1db90ae23..21f3a087e 100644 --- a/packages/sdk/package.json +++ b/packages/sdk/package.json @@ -80,10 +80,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..3c43db959 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 { helperScanCopies, referencesHelper } from './helper-reference.js'; /** * What a flow needs from the workspace it deploys into, read from inert @@ -157,8 +158,9 @@ export function flowRequirements( const text = typeof flow.body === 'function' ? Function.prototype.toString.call(flow.body) : ''; const root = contextParameter(text); if (root !== undefined) { + const { code, withStrings } = helperScanCopies(text); for (const { provider, namespace } of helperProviders) { - if (helperReference(root, namespace).test(text)) declare({ provider, from: 'helper', detail: `f.${namespace}` }); + if (referencesHelper(root, namespace, code, withStrings)) 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 @@ -225,11 +227,6 @@ function contextParameter(body: string): string | undefined { return (parameter?.[1] ?? parameter?.[2])?.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&'); } -/** 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'); -} - /** * The literal `to` of each `f.human(question, { to: "…" })` call in the body. * diff --git a/packages/sdk/src/helper-preflight.ts b/packages/sdk/src/helper-preflight.ts index 3f22d264a..c7ec294ea 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 { helperScanCopies, referencesHelper } from './helper-reference.js'; /** Static discovery never executes the body; dynamic aliases are checked at call time. */ export function preflightHelpers( @@ -11,9 +12,12 @@ export function preflightHelpers( const parameter = body.match(/^(?:async\s+)?(?:function(?:\s+[\w$]+)?\s*)?(?:\(\s*([\w$]+)|([\w$]+)\s*=>)/); const root = (parameter?.[1] ?? parameter?.[2])?.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const diagnostics: PreflightDiagnostic[] = []; + // Read as syntax, not text: a helper named inside a string or comment is not + // used, and refusing on one demands a mount the flow never touches. + const { code, withStrings } = helperScanCopies(body); 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)); + || (root !== undefined && referencesHelper(root, namespace, code, withStrings)); 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..c00e10f22 --- /dev/null +++ b/packages/sdk/src/helper-reference.ts @@ -0,0 +1,104 @@ +/** + * Does a flow body actually USE `f.`, read as syntax rather than as + * text? + * + * A body that merely mentions a helper in a comment or a string does not use + * it, and declaring a requirement from one refuses a flow for a mount it never + * needs. `f.run("echo f.gitlab")`, an agent prompt naming a helper, or a PR + * title in a commit message were each enough to demand a GitLab mount. + * + * Dot access is read on the copy with comments AND strings blanked. Bracket + * access needs the quoted key, so it is read on the comments-only copy and + * then confirmed against the fully blanked one: the `f[` before the key must + * have survived, which it does not inside a string. + * + * Mirrors Cloud's `flow-source-requirements.ts`, which already reads these + * shapes this way; the two are the same contract seen from either side, and + * the SDK being laxer meant a flow Cloud accepted could be refused locally. + */ +export function referencesHelper( + root: string, + namespace: string, + code: string, + withStrings: string, +): boolean { + const dot = new RegExp(`(?:^|[^\\w$.])${root}\\s*\\.\\s*${namespace}\\b`, 'u'); + if (dot.test(code)) return true; + const bracket = new RegExp(`(?:^|[^\\w$.])(${root}\\s*\\[\\s*)['"]${namespace}['"]\\s*\\]`, 'gu'); + for (const match of withStrings.matchAll(bracket)) { + const prefix = match[1]!; + const at = withStrings.indexOf(prefix, match.index!); + if (at !== -1 && code.slice(at, at + prefix.length) === prefix) return true; + } + return false; +} + +/** + * Blank comments, and optionally strings, to spaces of the same length so + * every index still lines up with the original source. + */ +export function blankKeepingLength(source: string, alsoStrings: boolean): string { + let out = ''; + let i = 0; + while (i < source.length) { + const ch = source[i]!; + const next = source[i + 1]; + const isComment = ch === '/' && (next === '/' || next === '*'); + const isString = ch === '"' || ch === "'" || ch === '`'; + if (isComment || (alsoStrings && isString)) { + const skipped = skipSpan(source, i); + if (skipped === -1) return out + ' '.repeat(source.length - i); + out += ' '.repeat(skipped - i); + i = skipped; + continue; + } + if (isString) { + const skipped = skipSpan(source, i); + if (skipped === -1) return out + source.slice(i); + out += source.slice(i, skipped); + i = skipped; + continue; + } + out += ch; + i += 1; + } + return out; +} + +/** Both copies a helper scan needs, from one walk of the source. */ +export function helperScanCopies(body: string): { code: string; withStrings: string } { + return { code: blankKeepingLength(body, true), withStrings: blankKeepingLength(body, false) }; +} + +/** Index just past the comment or string starting at `i`; `i` when none; -1 when unterminated. */ +function skipSpan(text: string, i: number): number { + const ch = text[i]!; + const next = text[i + 1]; + if (ch === '/' && next === '/') { const end = text.indexOf('\n', i); return end === -1 ? text.length : end + 1; } + if (ch === '/' && next === '*') { const end = text.indexOf('*/', i + 2); return end === -1 ? -1 : end + 2; } + if (ch === '"' || ch === "'" || ch === '`') { const end = stringEnd(text, i); return end === -1 ? -1 : end + 1; } + return i; +} + +/** Index of the quote closing the string opening at `start` (template `${…}` skipped); -1 if unterminated. */ +function stringEnd(text: string, start: number): number { + const quote = text[start]!; + let i = start + 1; + while (i < text.length) { + const ch = text[i]!; + if (ch === '\\') { i += 2; continue; } + if (ch === quote) return i; + if (quote === '`' && ch === '$' && text[i + 1] === '{') { + let depth = 1; + i += 2; + while (i < text.length && depth > 0) { + if (text[i] === '{') depth += 1; + else if (text[i] === '}') depth -= 1; + i += 1; + } + continue; + } + i += 1; + } + return -1; +} diff --git a/packages/sdk/tests/helper-reference.test.ts b/packages/sdk/tests/helper-reference.test.ts new file mode 100644 index 000000000..3c531631f --- /dev/null +++ b/packages/sdk/tests/helper-reference.test.ts @@ -0,0 +1,59 @@ +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'); + }); + + 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'); + }); +}); From f903a87bffe7f2c4f09e3e8e46613778a217e545 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Wed, 23 Sep 2026 22:31:26 -0700 Subject: [PATCH 2/6] fix(sdk): lex templates and regex literals in the helper scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found two shapes my blanking got wrong, and both were worse than the bug it fixed: they hid REAL helper use, so a flow would deploy without its mount and fail at runtime, where the original bug only refused a flow that needed nothing. await f.run(`echo ${await f.gitlab.issues.list({})}`) -> PASSED (regression) const a = /'/; await f.gitlab.issues.list({}) -> PASSED (regression) const p = /f.gitlab/ -> REFUSED (pre-existing) A template is not one span: quasis are text but each `${…}` is live code, so interpolations are now walked as code, nested templates included. A regex is not code: `/f.gitlab/` is a mention, and a quote inside one opened a phantom string that blanked the entire rest of the body. Regex-vs-division is resolved conservatively — only after a character that cannot end an expression. Misreading division as a regex would blank real code and hide a helper; misreading a regex as division only risks the milder false refusal, so ambiguity resolves toward "not a regex". Verified on all ten shapes (the three reported, the six already covered, plus division): regression test now 12 passing. Surface gate exits 0. The kernel-backed suites fail identically before and after (5/6 on tests/authored-helpers either way) — they need a live relayflowd. Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk/src/helper-reference.ts | 242 +++++++++++++++++--- packages/sdk/tests/helper-reference.test.ts | 32 +++ 2 files changed, 246 insertions(+), 28 deletions(-) diff --git a/packages/sdk/src/helper-reference.ts b/packages/sdk/src/helper-reference.ts index c00e10f22..5f7cbd72d 100644 --- a/packages/sdk/src/helper-reference.ts +++ b/packages/sdk/src/helper-reference.ts @@ -34,52 +34,238 @@ export function referencesHelper( } /** - * Blank comments, and optionally strings, to spaces of the same length so - * every index still lines up with the original source. + * Both scan copies, from one lexical walk, with every index preserved. + * + * `code` blanks comments, string literals and regex literals; `withStrings` + * blanks comments and regex literals but keeps string contents, because + * `f["gitlab"]` hides its namespace inside a string. + * + * Two shapes have to be lexed rather than skipped wholesale, and getting + * either wrong produces a FALSE NEGATIVE — a flow that really uses a helper + * deploying without its mount and failing at runtime, which is worse than the + * false refusal this scan exists to remove: + * + * - A template literal is not one span. Its quasis are text, but each + * `${...}` is live code that can call a helper, so interpolations are + * walked as code (nested templates, strings and comments included). + * - A regex literal is not code. `/f.gitlab/` is a mention, not a use, and + * a quote inside one (`/'/`) would otherwise open a phantom string and + * blank the entire rest of the body. */ -export function blankKeepingLength(source: string, alsoStrings: boolean): string { - let out = ''; +export function scanHelperSource(source: string): { code: string; withStrings: string } { + const code: string[] = []; + const withStrings: string[] = []; + const push = (text: string, blankCode: boolean, blankStrings: boolean): void => { + code.push(blankCode ? ' '.repeat(text.length) : text); + withStrings.push(blankStrings ? ' '.repeat(text.length) : text); + }; + let i = 0; while (i < source.length) { const ch = source[i]!; const next = source[i + 1]; - const isComment = ch === '/' && (next === '/' || next === '*'); - const isString = ch === '"' || ch === "'" || ch === '`'; - if (isComment || (alsoStrings && isString)) { - const skipped = skipSpan(source, i); - if (skipped === -1) return out + ' '.repeat(source.length - i); - out += ' '.repeat(skipped - i); - i = skipped; + + if (ch === '/' && next === '/') { + const end = source.indexOf('\n', i); + const stop = end === -1 ? source.length : end; + push(source.slice(i, stop), true, true); + i = stop; + continue; + } + if (ch === '/' && next === '*') { + const end = source.indexOf('*/', i + 2); + const stop = end === -1 ? source.length : end + 2; + push(source.slice(i, stop), true, true); + i = stop; + continue; + } + if (ch === '/' && regexCanStartAt(source, i)) { + const stop = regexEnd(source, i); + if (stop !== -1) { + // Keep the delimiters so the text still lexes as an expression; blank + // the body in BOTH copies — a helper named in a pattern is not used. + push('/', false, false); + push(source.slice(i + 1, stop - 1), true, true); + push(source.slice(stop - 1, stop), false, false); + i = stop; + continue; + } + } + if (ch === '"' || ch === "'") { + const stop = quotedEnd(source, i); + if (stop === -1) { push(source.slice(i), true, false); break; } + push(source.slice(i, stop), true, false); + i = stop; continue; } - if (isString) { - const skipped = skipSpan(source, i); - if (skipped === -1) return out + source.slice(i); - out += source.slice(i, skipped); - i = skipped; + if (ch === '`') { + i = walkTemplate(source, i, push); continue; } - out += ch; + push(ch, false, false); i += 1; } - return out; + return { code: code.join(''), withStrings: withStrings.join('') }; } -/** Both copies a helper scan needs, from one walk of the source. */ -export function helperScanCopies(body: string): { code: string; withStrings: string } { - return { code: blankKeepingLength(body, true), withStrings: blankKeepingLength(body, false) }; +/** + * Walk a template from its opening backtick, blanking quasis and recursing + * into `${...}` so helper calls inside an interpolation stay visible. + * Returns the index just past the closing backtick. + */ +function walkTemplate( + source: string, + start: number, + push: (text: string, blankCode: boolean, blankStrings: boolean) => void, +): number { + push('`', false, false); + let i = start + 1; + let quasi = i; + const flushQuasi = (stop: number): void => { + if (stop > quasi) push(source.slice(quasi, stop), true, false); + }; + while (i < source.length) { + const ch = source[i]!; + if (ch === '\\') { i += 2; continue; } + if (ch === '`') { + flushQuasi(i); + push('`', false, false); + return i + 1; + } + if (ch === '$' && source[i + 1] === '{') { + flushQuasi(i); + const close = expressionEnd(source, i + 2); + const stop = close === -1 ? source.length : close; + push('${', false, false); + const inner = scanHelperSource(source.slice(i + 2, stop)); + push('', false, false); + // Push the inner copies directly so both stay index-aligned. + pushPrescanned(push, inner, source.slice(i + 2, stop)); + if (close !== -1) push('}', false, false); + i = close === -1 ? source.length : close + 1; + quasi = i; + continue; + } + i += 1; + } + flushQuasi(i); + return i; +} + +/** Emit an already-scanned span, keeping `code` and `withStrings` distinct. */ +function pushPrescanned( + push: (text: string, blankCode: boolean, blankStrings: boolean) => void, + inner: { code: string; withStrings: string }, + raw: string, +): void { + // The two copies differ, so they cannot go through the shared `push`. + // Emit character-wise: identical characters keep their value, and a + // character blanked in one copy is emitted blanked there only. + for (let k = 0; k < raw.length; k += 1) { + const c = inner.code[k] ?? ' '; + const w = inner.withStrings[k] ?? ' '; + if (c === w) push(c, false, false); + else push(c === ' ' ? w : c, c === ' ', w === ' '); + } } -/** Index just past the comment or string starting at `i`; `i` when none; -1 when unterminated. */ -function skipSpan(text: string, i: number): number { - const ch = text[i]!; - const next = text[i + 1]; - if (ch === '/' && next === '/') { const end = text.indexOf('\n', i); return end === -1 ? text.length : end + 1; } - if (ch === '/' && next === '*') { const end = text.indexOf('*/', i + 2); return end === -1 ? -1 : end + 2; } - if (ch === '"' || ch === "'" || ch === '`') { const end = stringEnd(text, i); return end === -1 ? -1 : end + 1; } +/** Index just past the `}` closing an interpolation opened at `start`; -1 if unterminated. */ +function expressionEnd(source: string, start: number): number { + let depth = 1; + let i = start; + while (i < source.length) { + const ch = source[i]!; + if (ch === '"' || ch === "'") { const stop = quotedEnd(source, i); i = stop === -1 ? source.length : stop; continue; } + if (ch === '`') { i = skipTemplate(source, i); continue; } + if (ch === '/' && source[i + 1] === '/') { const end = source.indexOf('\n', i); i = end === -1 ? source.length : end; continue; } + if (ch === '/' && source[i + 1] === '*') { const end = source.indexOf('*/', i + 2); i = end === -1 ? source.length : end + 2; continue; } + if (ch === '/' && regexCanStartAt(source, i)) { const stop = regexEnd(source, i); if (stop !== -1) { i = stop; continue; } } + if (ch === '{') depth += 1; + else if (ch === '}') { depth -= 1; if (depth === 0) return i; } + i += 1; + } + return -1; +} + +/** Index just past the template closing backtick. */ +function skipTemplate(source: string, start: number): number { + let i = start + 1; + while (i < source.length) { + const ch = source[i]!; + if (ch === '\\') { i += 2; continue; } + if (ch === '`') return i + 1; + if (ch === '$' && source[i + 1] === '{') { + const close = expressionEnd(source, i + 2); + i = close === -1 ? source.length : close + 1; + continue; + } + i += 1; + } return i; } +/** Index just past the quote closing a '…' or "…" opened at `start`; -1 if unterminated. */ +function quotedEnd(source: string, start: number): number { + const quote = source[start]!; + let i = start + 1; + while (i < source.length) { + const ch = source[i]!; + if (ch === '\\') { i += 2; continue; } + if (ch === '\n') return -1; + if (ch === quote) return i + 1; + i += 1; + } + return -1; +} + +/** + * Whether the `/` at `i` opens a regex rather than dividing. + * + * Deliberately conservative: only after a character that cannot END an + * expression. Reading a division as a regex would blank real code and hide a + * helper (a false negative); reading a regex as division only risks the + * milder false refusal, so ambiguity resolves toward "not a regex". + */ +function regexCanStartAt(source: string, i: number): boolean { + let k = i - 1; + while (k >= 0 && /\s/u.test(source[k]!)) k -= 1; + if (k < 0) return true; + const prev = source[k]!; + if (/[)\]}]/u.test(prev)) return false; + if (/[\w$]/u.test(prev)) { + let start = k; + while (start >= 0 && /[\w$]/u.test(source[start]!)) start -= 1; + const word = source.slice(start + 1, k + 1); + return ['return', 'typeof', 'case', 'in', 'of', 'do', 'else', 'yield', 'await', 'void', 'delete', 'instanceof', 'new'].includes(word); + } + return true; +} + +/** Index just past the closing `/` (and flags) of a regex opened at `start`; -1 if not a regex. */ +function regexEnd(source: string, start: number): number { + let i = start + 1; + let inClass = false; + while (i < source.length) { + const ch = source[i]!; + if (ch === '\\') { i += 2; continue; } + if (ch === '\n') return -1; + if (inClass) { if (ch === ']') inClass = false; } + else if (ch === '[') inClass = true; + else if (ch === '/') { + i += 1; + while (i < source.length && /[a-z]/u.test(source[i]!)) i += 1; + return i; + } + i += 1; + } + return -1; +} + +/** Both copies a helper scan needs, from one walk of the source. */ +export function helperScanCopies(body: string): { code: string; withStrings: string } { + return scanHelperSource(body); +} + /** Index of the quote closing the string opening at `start` (template `${…}` skipped); -1 if unterminated. */ function stringEnd(text: string, start: number): number { const quote = text[start]!; diff --git a/packages/sdk/tests/helper-reference.test.ts b/packages/sdk/tests/helper-reference.test.ts index 3c531631f..5a47a9ab9 100644 --- a/packages/sdk/tests/helper-reference.test.ts +++ b/packages/sdk/tests/helper-reference.test.ts @@ -45,6 +45,38 @@ describe('helper references are read as syntax, not text', () => { .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([]); + }); + it('does not declare a requirement from a mentioned helper', () => { const mentioned = getFlowDefinition(flow('mention', async (ctx: any) => { await ctx.run('echo f.gitlab'); From 7019a6a25b077cb8e1be4f56fbd7ed76a50c7c9e Mon Sep 17 00:00:00 2001 From: Khaliq Date: Wed, 23 Sep 2026 22:49:21 -0700 Subject: [PATCH 3/6] refactor(sdk): parse the flow body instead of lexing it by hand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lexer this replaces had to decide whether `/` opened a regex or divided, which is not decidable without parse context — only a heuristic on the preceding token. Review was right that the heuristic was the weak point, and it was not merely theoretical: the lexer missed optional chaining outright, await f?.gitlab?.issues.list({}) -> PASSED, should refuse a false negative deploying a flow with no GitLab mount, to fail at the call. acorn (zero dependencies, ~565 KB unpacked) parses the body and the walk looks for member access on the context parameter. Regex-vs-division, templates and their interpolations, comments, strings and optional chaining are all settled by construction rather than by rule, so the whole class is gone rather than approximated. `Function.prototype.toString()` can yield an arrow, a function expression, a declaration or an object-literal method, and only some of those are expressions, so each shape gets a parse attempt. If none parse the scan falls back to matching text — deliberately the permissive direction, since under-reporting deploys a flow without a mount it needs while over-reporting only asks for one that may go unused. What no parser can see is indirection: `const p = 'gitlab'; f[p]…` needs value tracking. That is documented on the function, and it is why `header.tools[namespace]` — which already short-circuits this scan — is the authority and this is a convenience for the literal case. 17 tests now, including the five shapes that separate a parse from a lex. Surface gate exits 0. The kernel-backed suites fail identically before and after (5/7 on tests/authored-flow-slack either way). Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk/package-lock.json | 13 + packages/sdk/package.json | 1 + packages/sdk/src/flow-requirements.ts | 6 +- packages/sdk/src/helper-preflight.ts | 11 +- packages/sdk/src/helper-reference.ts | 355 +++++--------------- packages/sdk/tests/helper-reference.test.ts | 31 ++ 6 files changed, 146 insertions(+), 271 deletions(-) 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 21f3a087e..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", diff --git a/packages/sdk/src/flow-requirements.ts b/packages/sdk/src/flow-requirements.ts index 3c43db959..fbb890b79 100644 --- a/packages/sdk/src/flow-requirements.ts +++ b/packages/sdk/src/flow-requirements.ts @@ -4,7 +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 { helperScanCopies, referencesHelper } from './helper-reference.js'; +import { helperNamespacesUsed } from './helper-reference.js'; /** * What a flow needs from the workspace it deploys into, read from inert @@ -158,9 +158,9 @@ export function flowRequirements( const text = typeof flow.body === 'function' ? Function.prototype.toString.call(flow.body) : ''; const root = contextParameter(text); if (root !== undefined) { - const { code, withStrings } = helperScanCopies(text); + const referenced = helperNamespacesUsed(text, root); for (const { provider, namespace } of helperProviders) { - if (referencesHelper(root, namespace, code, withStrings)) 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 diff --git a/packages/sdk/src/helper-preflight.ts b/packages/sdk/src/helper-preflight.ts index c7ec294ea..dde16a8f4 100644 --- a/packages/sdk/src/helper-preflight.ts +++ b/packages/sdk/src/helper-preflight.ts @@ -1,6 +1,6 @@ import { helperProviders } from '@relayflows/surface/runtime'; import type { PreflightResult, PreflightDiagnostic } from './preflight.js'; -import { helperScanCopies, referencesHelper } from './helper-reference.js'; +import { helperNamespacesUsed } from './helper-reference.js'; /** Static discovery never executes the body; dynamic aliases are checked at call time. */ export function preflightHelpers( @@ -12,12 +12,13 @@ export function preflightHelpers( const parameter = body.match(/^(?:async\s+)?(?:function(?:\s+[\w$]+)?\s*)?(?:\(\s*([\w$]+)|([\w$]+)\s*=>)/); const root = (parameter?.[1] ?? parameter?.[2])?.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const diagnostics: PreflightDiagnostic[] = []; - // Read as syntax, not text: a helper named inside a string or comment is not - // used, and refusing on one demands a mount the flow never touches. - const { code, withStrings } = helperScanCopies(body); + // 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 && referencesHelper(root, namespace, code, withStrings)); + || 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 index 5f7cbd72d..d5ff83133 100644 --- a/packages/sdk/src/helper-reference.ts +++ b/packages/sdk/src/helper-reference.ts @@ -1,290 +1,119 @@ -/** - * Does a flow body actually USE `f.`, read as syntax rather than as - * text? - * - * A body that merely mentions a helper in a comment or a string does not use - * it, and declaring a requirement from one refuses a flow for a mount it never - * needs. `f.run("echo f.gitlab")`, an agent prompt naming a helper, or a PR - * title in a commit message were each enough to demand a GitLab mount. - * - * Dot access is read on the copy with comments AND strings blanked. Bracket - * access needs the quoted key, so it is read on the comments-only copy and - * then confirmed against the fully blanked one: the `f[` before the key must - * have survived, which it does not inside a string. - * - * Mirrors Cloud's `flow-source-requirements.ts`, which already reads these - * shapes this way; the two are the same contract seen from either side, and - * the SDK being laxer meant a flow Cloud accepted could be refused locally. - */ -export function referencesHelper( - root: string, - namespace: string, - code: string, - withStrings: string, -): boolean { - const dot = new RegExp(`(?:^|[^\\w$.])${root}\\s*\\.\\s*${namespace}\\b`, 'u'); - if (dot.test(code)) return true; - const bracket = new RegExp(`(?:^|[^\\w$.])(${root}\\s*\\[\\s*)['"]${namespace}['"]\\s*\\]`, 'gu'); - for (const match of withStrings.matchAll(bracket)) { - const prefix = match[1]!; - const at = withStrings.indexOf(prefix, match.index!); - if (at !== -1 && code.slice(at, at + prefix.length) === prefix) return true; - } - return false; -} +import { parse, parseExpressionAt } from 'acorn'; /** - * Both scan copies, from one lexical walk, with every index preserved. + * Which `f.` helpers a flow body actually USES. * - * `code` blanks comments, string literals and regex literals; `withStrings` - * blanks comments and regex literals but keeps string contents, because - * `f["gitlab"]` hides its namespace inside a string. + * 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. * - * Two shapes have to be lexed rather than skipped wholesale, and getting - * either wrong produces a FALSE NEGATIVE — a flow that really uses a helper - * deploying without its mount and failing at runtime, which is worse than the - * false refusal this scan exists to remove: + * 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. * - * - A template literal is not one span. Its quasis are text, but each - * `${...}` is live code that can call a helper, so interpolations are - * walked as code (nested templates, strings and comments included). - * - A regex literal is not code. `/f.gitlab/` is a mention, not a use, and - * a quote inside one (`/'/`) would otherwise open a phantom string and - * blank the entire rest of the body. + * 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 scanHelperSource(source: string): { code: string; withStrings: string } { - const code: string[] = []; - const withStrings: string[] = []; - const push = (text: string, blankCode: boolean, blankStrings: boolean): void => { - code.push(blankCode ? ' '.repeat(text.length) : text); - withStrings.push(blankStrings ? ' '.repeat(text.length) : text); - }; - - let i = 0; - while (i < source.length) { - const ch = source[i]!; - const next = source[i + 1]; +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; +} - if (ch === '/' && next === '/') { - const end = source.indexOf('\n', i); - const stop = end === -1 ? source.length : end; - push(source.slice(i, stop), true, true); - i = stop; - continue; - } - if (ch === '/' && next === '*') { - const end = source.indexOf('*/', i + 2); - const stop = end === -1 ? source.length : end + 2; - push(source.slice(i, stop), true, true); - i = stop; - continue; - } - if (ch === '/' && regexCanStartAt(source, i)) { - const stop = regexEnd(source, i); - if (stop !== -1) { - // Keep the delimiters so the text still lexes as an expression; blank - // the body in BOTH copies — a helper named in a pattern is not used. - push('/', false, false); - push(source.slice(i + 1, stop - 1), true, true); - push(source.slice(stop - 1, stop), false, false); - i = stop; - continue; - } - } - if (ch === '"' || ch === "'") { - const stop = quotedEnd(source, i); - if (stop === -1) { push(source.slice(i), true, false); break; } - push(source.slice(i, stop), true, false); - i = stop; - continue; - } - if (ch === '`') { - i = walkTemplate(source, i, push); - continue; - } - push(ch, false, false); - i += 1; - } - return { code: code.join(''), withStrings: withStrings.join('') }; +/** `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; } /** - * Walk a template from its opening backtick, blanking quasis and recursing - * into `${...}` so helper calls inside an interpolation stay visible. - * Returns the index just past the closing backtick. + * 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 walkTemplate( - source: string, - start: number, - push: (text: string, blankCode: boolean, blankStrings: boolean) => void, -): number { - push('`', false, false); - let i = start + 1; - let quasi = i; - const flushQuasi = (stop: number): void => { - if (stop > quasi) push(source.slice(quasi, stop), true, false); - }; - while (i < source.length) { - const ch = source[i]!; - if (ch === '\\') { i += 2; continue; } - if (ch === '`') { - flushQuasi(i); - push('`', false, false); - return i + 1; - } - if (ch === '$' && source[i + 1] === '{') { - flushQuasi(i); - const close = expressionEnd(source, i + 2); - const stop = close === -1 ? source.length : close; - push('${', false, false); - const inner = scanHelperSource(source.slice(i + 2, stop)); - push('', false, false); - // Push the inner copies directly so both stay index-aligned. - pushPrescanned(push, inner, source.slice(i + 2, stop)); - if (close !== -1) push('}', false, false); - i = close === -1 ? source.length : close + 1; - quasi = i; +function parseFlowBody(body: string): AstNode | null { + const options = { ecmaVersion: 'latest' as const, allowAwaitOutsideFunction: true, allowReturnOutsideFunction: true }; + for (const attempt of [ + () => parseExpressionAt(body, 0, options) as unknown as AstNode, + () => 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. + () => parseExpressionAt(`({${body}})`, 0, options) as unknown as AstNode, + ]) { + try { + return attempt(); + } catch { continue; } - i += 1; } - flushQuasi(i); - return i; -} - -/** Emit an already-scanned span, keeping `code` and `withStrings` distinct. */ -function pushPrescanned( - push: (text: string, blankCode: boolean, blankStrings: boolean) => void, - inner: { code: string; withStrings: string }, - raw: string, -): void { - // The two copies differ, so they cannot go through the shared `push`. - // Emit character-wise: identical characters keep their value, and a - // character blanked in one copy is emitted blanked there only. - for (let k = 0; k < raw.length; k += 1) { - const c = inner.code[k] ?? ' '; - const w = inner.withStrings[k] ?? ' '; - if (c === w) push(c, false, false); - else push(c === ' ' ? w : c, c === ' ', w === ' '); - } -} - -/** Index just past the `}` closing an interpolation opened at `start`; -1 if unterminated. */ -function expressionEnd(source: string, start: number): number { - let depth = 1; - let i = start; - while (i < source.length) { - const ch = source[i]!; - if (ch === '"' || ch === "'") { const stop = quotedEnd(source, i); i = stop === -1 ? source.length : stop; continue; } - if (ch === '`') { i = skipTemplate(source, i); continue; } - if (ch === '/' && source[i + 1] === '/') { const end = source.indexOf('\n', i); i = end === -1 ? source.length : end; continue; } - if (ch === '/' && source[i + 1] === '*') { const end = source.indexOf('*/', i + 2); i = end === -1 ? source.length : end + 2; continue; } - if (ch === '/' && regexCanStartAt(source, i)) { const stop = regexEnd(source, i); if (stop !== -1) { i = stop; continue; } } - if (ch === '{') depth += 1; - else if (ch === '}') { depth -= 1; if (depth === 0) return i; } - i += 1; - } - return -1; -} - -/** Index just past the template closing backtick. */ -function skipTemplate(source: string, start: number): number { - let i = start + 1; - while (i < source.length) { - const ch = source[i]!; - if (ch === '\\') { i += 2; continue; } - if (ch === '`') return i + 1; - if (ch === '$' && source[i + 1] === '{') { - const close = expressionEnd(source, i + 2); - i = close === -1 ? source.length : close + 1; - continue; - } - i += 1; - } - return i; -} - -/** Index just past the quote closing a '…' or "…" opened at `start`; -1 if unterminated. */ -function quotedEnd(source: string, start: number): number { - const quote = source[start]!; - let i = start + 1; - while (i < source.length) { - const ch = source[i]!; - if (ch === '\\') { i += 2; continue; } - if (ch === '\n') return -1; - if (ch === quote) return i + 1; - i += 1; - } - return -1; + return null; } /** - * Whether the `/` at `i` opens a regex rather than dividing. + * Last resort when nothing parses: match the text. * - * Deliberately conservative: only after a character that cannot END an - * expression. Reading a division as a regex would blank real code and hide a - * helper (a false negative); reading a regex as division only risks the - * milder false refusal, so ambiguity resolves toward "not a regex". + * 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 regexCanStartAt(source: string, i: number): boolean { - let k = i - 1; - while (k >= 0 && /\s/u.test(source[k]!)) k -= 1; - if (k < 0) return true; - const prev = source[k]!; - if (/[)\]}]/u.test(prev)) return false; - if (/[\w$]/u.test(prev)) { - let start = k; - while (start >= 0 && /[\w$]/u.test(source[start]!)) start -= 1; - const word = source.slice(start + 1, k + 1); - return ['return', 'typeof', 'case', 'in', 'of', 'do', 'else', 'yield', 'await', 'void', 'delete', 'instanceof', 'new'].includes(word); +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 true; + return used; } -/** Index just past the closing `/` (and flags) of a regex opened at `start`; -1 if not a regex. */ -function regexEnd(source: string, start: number): number { - let i = start + 1; - let inClass = false; - while (i < source.length) { - const ch = source[i]!; - if (ch === '\\') { i += 2; continue; } - if (ch === '\n') return -1; - if (inClass) { if (ch === ']') inClass = false; } - else if (ch === '[') inClass = true; - else if (ch === '/') { - i += 1; - while (i < source.length && /[a-z]/u.test(source[i]!)) i += 1; - return i; +type AstNode = { + type: string; + 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); } - i += 1; } - return -1; } -/** Both copies a helper scan needs, from one walk of the source. */ -export function helperScanCopies(body: string): { code: string; withStrings: string } { - return scanHelperSource(body); -} - -/** Index of the quote closing the string opening at `start` (template `${…}` skipped); -1 if unterminated. */ -function stringEnd(text: string, start: number): number { - const quote = text[start]!; - let i = start + 1; - while (i < text.length) { - const ch = text[i]!; - if (ch === '\\') { i += 2; continue; } - if (ch === quote) return i; - if (quote === '`' && ch === '$' && text[i + 1] === '{') { - let depth = 1; - i += 2; - while (i < text.length && depth > 0) { - if (text[i] === '{') depth += 1; - else if (text[i] === '}') depth -= 1; - i += 1; - } - continue; - } - i += 1; - } - return -1; +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 index 5a47a9ab9..9e3a5397d 100644 --- a/packages/sdk/tests/helper-reference.test.ts +++ b/packages/sdk/tests/helper-reference.test.ts @@ -77,6 +77,37 @@ describe('helper references are read as syntax, not text', () => { 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([]); + }); + it('does not declare a requirement from a mentioned helper', () => { const mentioned = getFlowDefinition(flow('mention', async (ctx: any) => { await ctx.run('echo f.gitlab'); From 70dc7a57379a68158d14fa2b2e6d7e3643d7aedd Mon Sep 17 00:00:00 2001 From: Khaliq Date: Wed, 23 Sep 2026 23:00:19 -0700 Subject: [PATCH 4/6] fix(sdk): unescape the context parameter and require a whole-body parse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more false negatives from review, both hiding real helper use. `root` was regex-escaped from when it built a pattern, but it is now compared to an AST Identifier name. A legal parameter like `f$` became `f\$`, matched no identifier, and every helper call in that flow went undeclared. `contextParameter` now returns the raw name and the regex scanners escape it themselves. `parseExpressionAt` stops at the end of the first expression without objecting to what follows, so `async post(f) { … }` parsed as the identifier `async`, reported success having read five characters, and declared nothing. Each attempt must now consume the whole source. Fixing that exposed the same case failing one step earlier: the parameter regex did not match method shorthand at all, so `root` was undefined and the body was never scanned — true on main too, not a regression. It now matches, so the case works end to end rather than only past the parser. 33 tests. Surface gate exits 0. Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk/src/flow-requirements.ts | 30 ++++++++++++++++----- packages/sdk/src/helper-preflight.ts | 6 +++-- packages/sdk/src/helper-reference.ts | 17 +++++++++--- packages/sdk/tests/helper-reference.test.ts | 15 +++++++++++ 4 files changed, 57 insertions(+), 11 deletions(-) diff --git a/packages/sdk/src/flow-requirements.ts b/packages/sdk/src/flow-requirements.ts index fbb890b79..f1363f283 100644 --- a/packages/sdk/src/flow-requirements.ts +++ b/packages/sdk/src/flow-requirements.ts @@ -156,9 +156,12 @@ 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 referenced = helperNamespacesUsed(text, root); + 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 (referenced.has(namespace)) declare({ provider, from: 'helper', detail: `f.${namespace}` }); } @@ -221,10 +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*=>|[\w$]+\s*\(\s*([\w$]+))/u); + return parameter?.[1] ?? parameter?.[2] ?? parameter?.[3]; +} + +/** 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 dde16a8f4..f9b148273 100644 --- a/packages/sdk/src/helper-preflight.ts +++ b/packages/sdk/src/helper-preflight.ts @@ -9,8 +9,10 @@ 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*=>|[\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 diff --git a/packages/sdk/src/helper-reference.ts b/packages/sdk/src/helper-reference.ts index d5ff83133..fa7d1bf2e 100644 --- a/packages/sdk/src/helper-reference.ts +++ b/packages/sdk/src/helper-reference.ts @@ -56,13 +56,23 @@ function memberName(node: AstNode): string | undefined { */ 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 [ - () => parseExpressionAt(body, 0, options) as unknown as AstNode, - () => parseExpressionAt(`(${body})`, 0, options) as unknown as AstNode, + () => 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. - () => parseExpressionAt(`({${body}})`, 0, options) as unknown as AstNode, + () => whole(`({${body}})`, parseExpressionAt(`({${body}})`, 0, options) as unknown as AstNode), ]) { try { return attempt(); @@ -94,6 +104,7 @@ function textFallback(body: string, root: string): ReadonlySet { type AstNode = { type: string; + end?: number; name?: string; value?: unknown; computed?: boolean; diff --git a/packages/sdk/tests/helper-reference.test.ts b/packages/sdk/tests/helper-reference.test.ts index 9e3a5397d..c0ceed972 100644 --- a/packages/sdk/tests/helper-reference.test.ts +++ b/packages/sdk/tests/helper-reference.test.ts @@ -108,6 +108,21 @@ describe('helper references are read as syntax, not text', () => { 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('does not declare a requirement from a mentioned helper', () => { const mentioned = getFlowDefinition(flow('mention', async (ctx: any) => { await ctx.run('echo f.gitlab'); From dc51fcd861375cebcc19ebe4aab43a12ff67107f Mon Sep 17 00:00:00 2001 From: Khaliq Date: Wed, 23 Sep 2026 23:06:10 -0700 Subject: [PATCH 5/6] fix(sdk): recognise generator method bodies as a context parameter shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checking the non-async method case from review surfaced a third shape the parameter regex still missed: a generator method (`*gen(f) { … }`), whose leading `*` kept it from matching at all, leaving `root` undefined and the body unscanned. Same class as the method-shorthand gap, same consequence — a helper used and never declared. Non-async methods and `$`-prefixed parameters were already correct after 70dc7a57; both are now pinned by tests so the shapes stay covered. 34 tests. Surface gate exits 0. Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk/src/flow-requirements.ts | 2 +- packages/sdk/src/helper-preflight.ts | 2 +- packages/sdk/tests/helper-reference.test.ts | 9 +++++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/sdk/src/flow-requirements.ts b/packages/sdk/src/flow-requirements.ts index f1363f283..1f381a62e 100644 --- a/packages/sdk/src/flow-requirements.ts +++ b/packages/sdk/src/flow-requirements.ts @@ -236,7 +236,7 @@ function contextParameter(body: string): string | undefined { // 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*=>|[\w$]+\s*\(\s*([\w$]+))/u); + 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]; } diff --git a/packages/sdk/src/helper-preflight.ts b/packages/sdk/src/helper-preflight.ts index f9b148273..112192c2a 100644 --- a/packages/sdk/src/helper-preflight.ts +++ b/packages/sdk/src/helper-preflight.ts @@ -9,7 +9,7 @@ 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*\*?\s*(?:[\w$]+)?\s*)?(?:\(\s*([\w$]+)|([\w$]+)\s*=>|[\w$]+\s*\(\s*([\w$]+))/u); + 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]; diff --git a/packages/sdk/tests/helper-reference.test.ts b/packages/sdk/tests/helper-reference.test.ts index c0ceed972..294122958 100644 --- a/packages/sdk/tests/helper-reference.test.ts +++ b/packages/sdk/tests/helper-reference.test.ts @@ -123,6 +123,15 @@ describe('helper references are read as syntax, not text', () => { 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'); + }); + it('does not declare a requirement from a mentioned helper', () => { const mentioned = getFlowDefinition(flow('mention', async (ctx: any) => { await ctx.run('echo f.gitlab'); From 471e5ba4a5c4524867c724fbc92fe1169482b959 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Wed, 23 Sep 2026 23:10:55 -0700 Subject: [PATCH 6/6] test(sdk): cover the flowRequirements parameter path, not just preflight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught that the generator-method fix was only asserted through `preflightHelpers`, while the identical regex also lives in `flowRequirements`, which extracts the context parameter itself. Verified the gap by mutation: reverting the flow-requirements.ts line left all 34 tests green, so that mirror was pinned by nothing. Adds a flowRequirements case per body shape — async method, plain method, generator method, `$`-prefixed parameter, and a string mention that must NOT declare. Re-ran the same mutation with it in place: the revert now fails that test and passes once restored, so the coverage is real rather than decorative. Co-Authored-By: Claude Opus 5 (1M context) --- packages/sdk/tests/helper-reference.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/sdk/tests/helper-reference.test.ts b/packages/sdk/tests/helper-reference.test.ts index 294122958..bfcfcade8 100644 --- a/packages/sdk/tests/helper-reference.test.ts +++ b/packages/sdk/tests/helper-reference.test.ts @@ -132,6 +132,24 @@ describe('helper references are read as syntax, not text', () => { 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');