Skip to content

fix(sdk): read helper references as syntax, and drop the surface peer override - #572

Merged
khaliqgant merged 6 commits into
mainfrom
chore/drop-surface-peer-override
Sep 24, 2026
Merged

khaliqgant merged 6 commits into
mainfrom
chore/drop-surface-peer-override

Conversation

@khaliqgant

@khaliqgant khaliqgant commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Two cleanups, both unblocked by surface 2.0.30.

1. A helper named is not a helper used

Both readers tested the raw body text for f.<namespace>, so a mere mention declared a requirement and refused the flow:

f.run("echo f.gitlab")                                  → REFUSED helper_provider.mount_required
f.agent("a", { task: "explain how f.slack.post works" }) → REFUSED
/* f.gitlab */                                           → REFUSED

None of those touch a helper. The refusal demanded a relayfile mount the flow never uses — and it made the SDK stricter than Cloud, whose flow-source-requirements.ts already reads these shapes as syntax, so a flow Cloud accepts could be refused locally.

Now: dot access is read on the copy with comments and strings blanked; bracket access (f["gitlab"], whose key is a string) on the comments-only copy, then confirmed against the fully blanked one so the f[ must have survived. Real usage still refuses, verified in both forms.

Extracted into helper-reference.ts because preflightHelpers and flowRequirements each carried their own copy of the regex. The comment in flow-requirements.ts literally said "Same recognition as preflightHelpers" — which is how the two drifted into the same bug twice, and why fixing one first didn't change the CLI's behaviour at all.

2. Drop the overrides entry from #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 inert. #537 said it "becomes a no-op once a surface carrying peer 0.4.12 is published and should be deleted then" — this is that.

Verified rather than assumed: npm install and a clean npm ci both exit 0 without it, with the @relayfile/sdk peer still installed, and scripts/surface-package-gate.sh exits 0.

Checks

  • New regression test asserts both directions; red against main's code (2 failed), green here (6 passed).
  • scripts/surface-package-gate.sh → exit 0, PACKED_RUNTIME_OK, PACKED_TYPESCRIPT_OK, surface 52/52, SDK 34/34.
  • 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, so CI is the real signal there.

🤖 Generated with Claude Code


Note

Medium Risk
Changes deploy-time preflight and static requirement inference for all flows; incorrect detection could refuse valid deploys or miss required mounts, though tests cover both false-positive and false-negative cases.

Overview
Helper mount detection now parses flow bodies instead of scanning raw text, so mentions in strings, comments, template text, or regex literals no longer trigger false mount_required refusals or integration requirements. Real f.<namespace> access (including optional chaining, template interpolations, and bracket literals) is still detected via a shared helper-reference.ts module using acorn, with a permissive regex fallback when parsing fails.

Context-parameter handling is fixed for both preflightHelpers and flowRequirements: the first parameter name is returned unescaped for AST comparison (fixing flows whose parameter contains $ or other metacharacters), and the extraction regex now covers object-literal method bodies and generator methods so those shapes are scanned instead of skipped.

Dependency cleanup: adds acorn to @relayflows/sdk and removes the obsolete package.json overrides entry for @relayflows/surface / @relayfile/relay-helpers now that surface 2.0.30 aligns peers. Regression tests cover both preflight and flowRequirements.

Reviewed by Cursor Bugbot for commit 471e5ba. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Reads helper references as syntax so a helper merely named in a string, comment, template text, or regex no longer demands a mount, and drops a now-inert peer override in packages/sdk/package.json.

Bug Fixes

  • The scan now parses the flow body with acorn instead of lexing it by hand, so regex-vs-division, templates, and optional chaining are settled by construction; a text fallback only runs when nothing parses.
  • The context parameter is no longer regex-escaped — an escaped name matched no AST identifier and hid every helper call, undeclaring a mount a flow really needed.
  • Parse attempts must now consume the whole body; parseExpressionAt read an object-literal method body as its async keyword and skipped its helper calls.
  • The context-parameter regex also matches generator and non-async method bodies (*gen(f), plain(f)), further Function.prototype.toString() shapes that previously left the body unscanned.
  • Recognition moved to helper-reference.ts, shared by preflightHelpers and flowRequirements; regression tests cover both directions.

Dependencies

  • Adds acorn and removes the overrides entry forcing @relayfile/relay-helpers; surface 2.0.30 ships peer 0.4.12, so it was a no-op.

Written for commit dc51fcd. Summary will update on new commits.

Review in cubic

… override

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) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 51e7da83-0764-487d-aaeb-95f668dfd57c

📥 Commits

Reviewing files that changed from the base of the PR and between e07a190 and 471e5ba.

⛔ Files ignored due to path filters (1)
  • packages/sdk/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • packages/sdk/package.json
  • packages/sdk/src/flow-requirements.ts
  • packages/sdk/src/helper-preflight.ts
  • packages/sdk/src/helper-reference.ts
  • packages/sdk/tests/helper-reference.test.ts
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-24T05:24:48.720740Z 5357164 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Devin Review

Comment thread packages/sdk/src/helper-reference.ts Outdated
Comment on lines +48 to +53
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Template helper calls bypass mount checks

When a template interpolates f.gitlab, blankKeepingLength erases the executable expression along with the template text. preflightHelpers then accepts a flow without the required mount, and flowRequirements omits its integration.

Learn more

A template literal contains ordinary string segments and executable ${...} expressions. preflightHelpers and flowRequirements both scan the copy produced here to discover required integration mounts. Blanking the entire template hides helper calls inside interpolations, so neither reader declares that need before execution.

Example: For async f => { await f.run(echo ${await f.gitlab.issues.list({})}); }, the interpolation invokes GitLab, but the scan replaces everything between the backticks with spaces. With no GitLab mount, the helper preflight succeeds instead of refusing.

Recommended fix: Preserve executable template expressions while blanking only the literal template segments. Walk nested braces, strings, comments and nested templates within ${...}, keeping both scan copies aligned by source index; add interpolation cases for dot and bracket helper calls to both readers' tests.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in f903a87b — and you were right that this is the serious direction. I verified it before fixing: await f.run(\echo ${await f.gitlab.issues.list({})}`)` passed preflight, so a flow really using GitLab would have deployed with no mount and failed at runtime. That is a regression I introduced; the bug I was fixing only produced a false refusal.

A template is no longer one span: quasis are blanked as text, each ${…} is walked as code, nested templates included. Covered by tests for a single and a nested interpolation, plus one asserting a helper named in template TEXT still passes.

Comment thread packages/sdk/src/helper-reference.ts Outdated
Comment on lines +46 to +49
const isComment = ch === '/' && (next === '/' || next === '*');
const isString = ch === '"' || ch === "'" || ch === '`';
if (isComment || (alsoStrings && isString)) {
const skipped = skipSpan(source, i);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Regex literals trigger false mount refusals

When a body contains /f.gitlab/, blankKeepingLength leaves the regex text intact. preflightHelpers reads it as helper use and refuses a flow that never touches GitLab.

Learn more

The scanner only skips comments and quoted strings, not JavaScript regular expression literals. referencesHelper then tests the surviving regex source as though it were code, so both preflight and requirement discovery identify a helper that is not used.

Example: async f => { const pattern = /f.gitlab/; await f.run('true'); } invokes no GitLab helper. With no GitLab mount, preflight reports helper_provider.mount_required and blocks the flow.

Recommended fix: Recognize regular expression literals during the lexical walk, including escapes and character classes, and blank their contents while preserving indices. Distinguish a regex start from division, then test a regex mention alongside real helper access following a regex.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in f903a87b. Both halves reproduced:

  • const p = /f.gitlab/ → REFUSED (false positive, pre-existing)
  • const a = /'/; await f.gitlab.issues.list({}) → PASSED (false negative, a regression I introduced — the quote opened a phantom string that blanked the rest of the body)

Regex literals are now lexed: delimiters kept so the text still reads as an expression, body blanked in both copies. Regex-vs-division is deliberately conservative — a regex is only recognised after a character that cannot end an expression, because misreading division as a regex would blank real code and hide a helper, while misreading a regex as division only risks the milder false refusal. const n = 10 / 2 is covered by a test.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5357164400

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/sdk/src/helper-reference.ts Outdated
Comment on lines +91 to +94
if (quote === '`' && ch === '$' && text[i + 1] === '{') {
let depth = 1;
i += 2;
while (i < text.length && depth > 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Scan code inside template interpolations

When a helper is invoked from a template interpolation, such as `${await f.gitlab.issues.list({})}`, this branch skips the interpolation and blankKeepingLength blanks the entire template. Consequently, both preflightHelpers and flowRequirements conclude that GitLab is unused, allowing a deployment without the required mount to pass preflight and fail only when the body executes. Interpolation expressions need to remain visible to the syntax scan.

AGENTS.md reference: AGENTS.md:L16-L18

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in f903a87b — and you were right that this is the serious direction. I verified it before fixing: await f.run(\echo ${await f.gitlab.issues.list({})}`)` passed preflight, so a flow really using GitLab would have deployed with no mount and failed at runtime. That is a regression I introduced; the bug I was fixing only produced a false refusal.

A template is no longer one span: quasis are blanked as text, each ${…} is walked as code, nested templates included. Covered by tests for a single and a nested interpolation, plus one asserting a helper named in template TEXT still passes.

Comment thread packages/sdk/src/helper-reference.ts Outdated
const ch = source[i]!;
const next = source[i + 1];
const isComment = ch === '/' && (next === '/' || next === '*');
const isString = ch === '"' || ch === "'" || ch === '`';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Distinguish regex contents from string literals

For a valid body containing a regex with a quote before a real helper call, such as const apostrophe = /'/; await f.gitlab.issues.list({}), the quote inside the regex is treated as the start of a string; with no later matching quote, the remainder of the body is blanked. This newly hides the real helper call from both requirement readers, so a missing GitLab mount passes preflight and becomes a runtime failure. The scanner must recognize regular-expression literals rather than treating every quote as source-level string syntax.

AGENTS.md reference: AGENTS.md:L16-L18

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in f903a87b. Both halves reproduced:

  • const p = /f.gitlab/ → REFUSED (false positive, pre-existing)
  • const a = /'/; await f.gitlab.issues.list({}) → PASSED (false negative, a regression I introduced — the quote opened a phantom string that blanked the rest of the body)

Regex literals are now lexed: delimiters kept so the text still reads as an expression, body blanked in both copies. Regex-vs-division is deliberately conservative — a regex is only recognised after a character that cannot end an expression, because misreading division as a regex would blank real code and hide a helper, while misreading a regex as division only risks the milder false refusal. const n = 10 / 2 is covered by a test.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread packages/sdk/src/helper-reference.ts Outdated
Comment thread packages/sdk/src/helper-reference.ts Outdated
const ch = source[i]!;
const next = source[i + 1];
const isComment = ch === '/' && (next === '/' || next === '*');
const isString = ch === '"' || ch === "'" || ch === '`';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex quotes hide later helpers

Medium Severity

blankKeepingLength only treats comments and quotes as spans, so a regex literal is walked as code. A quote inside that literal opens a phantom string and can blank the rest of the body, hiding later real helper references from both readers.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5357164. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in f903a87b. Both halves reproduced:

  • const p = /f.gitlab/ → REFUSED (false positive, pre-existing)
  • const a = /'/; await f.gitlab.issues.list({}) → PASSED (false negative, a regression I introduced — the quote opened a phantom string that blanked the rest of the body)

Regex literals are now lexed: delimiters kept so the text still reads as an expression, body blanked in both copies. Regex-vs-division is deliberately conservative — a regex is only recognised after a character that cannot end an expression, because misreading division as a regex would blank real code and hide a helper, while misreading a regex as division only risks the milder false refusal. const n = 10 / 2 is covered by a test.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 5 files

You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/sdk/src/flow-requirements.ts">

<violation number="1" location="packages/sdk/src/flow-requirements.ts:161">
P1: Template interpolations are executable code, but `helperScanCopies(text)` removes them along with template text. A flow such as `async f => `${f.gitlab}`` can therefore omit the GitLab requirement and skip the needed integration or mount validation; scan `${...}` expressions while ignoring only literal template portions.</violation>
</file>

Comment thread packages/sdk/src/flow-requirements.ts Outdated
Comment thread packages/sdk/src/helper-reference.ts Outdated
Comment thread packages/sdk/src/helper-reference.ts Outdated
khaliqgant and others added 2 commits September 23, 2026 22:31
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@khaliqgant

Copy link
Copy Markdown
Member Author

Replaced the hand-rolled lexer with a real parse in 7019a6a2 — the heuristic was the right thing to object to.

Why the lexer could not be made right. Whether / opens a regex or divides depends on whether the preceding token ends an expression, which is not decidable without parse context. I had it resolving ambiguity conservatively, but that is still a rule approximating a grammar. It was not only theoretical either — the lexer missed optional chaining outright:

await f?.gitlab?.issues.list({})   -> PASSED, should refuse

A false negative: that flow deploys with no GitLab mount and fails at the call. Measured against the previous commit, not assumed.

What replaced it. acorn (zero dependencies, ~565 KB unpacked) parses the body; the walk looks for member access on the context parameter. Regex-vs-division, templates and their interpolations, comments, strings and optional chaining are settled by construction rather than by rule.

Function.prototype.toString() can return an arrow, a function expression, a declaration, or an object-literal method — only some are expressions — so each shape gets a parse attempt, with a text-match fallback if none parse. That fallback is deliberately the permissive direction: under-reporting deploys a flow without a mount it needs; over-reporting only asks for one that may go unused.

The remaining ceiling, which no parser fixes. const p = 'gitlab'; f[p].issues… needs value tracking, not parsing, and passes today. So this scan is a convenience for the literal case and cannot be the authority — header.tools[namespace], which already short-circuits it, is. That is now documented on the function rather than implied.

Note for review: this adds acorn as a runtime dependency of the published SDK. That is a supply-chain call that is yours, not mine — if you would rather not take it, the previous commit (f903a87b) is the lexer version and is strictly better than main, just not exact.

17 tests, including the five shapes that separate a parse from a lex. Surface gate exits 0; kernel-backed suites fail identically before and after.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 6 files (changes from recent commits).

You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/sdk/src/helper-reference.ts Outdated
Comment thread packages/sdk/src/helper-preflight.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7019a6a. Configure here.

Comment thread packages/sdk/src/helper-reference.ts
Comment thread packages/sdk/src/helper-reference.ts Outdated
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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial parse hides helper use

Medium Severity

parseExpressionAt accepts a leading prefix and is tried first, so a non-async method body is read as a call and the later object-literal attempt never runs. Helper use inside the method is missed, and the scan does not fall back to text.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7019a6a. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed in 70dc7a57 — every parse attempt must now consume the whole source, so a method body can no longer be read as its prefix.

Your non-async framing was the useful part: I had only tested async post(f). Checking plain(f) { … } confirmed it works, but the same test surfaced a third shape still broken — a generator method *gen(f) { … }, whose leading * kept the parameter regex from matching at all, so root was undefined and the body went unscanned. Fixed in dc51fcd8. All three method shapes plus $-prefixed parameters are now pinned by tests.

khaliqgant and others added 2 commits September 23, 2026 23:00
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) <noreply@anthropic.com>
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
70dc7a5; 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) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files (changes from recent commits).

You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/sdk/src/flow-requirements.ts">

<violation number="1" location="packages/sdk/src/flow-requirements.ts:239">
P3: The regex change itself is correct (generator-method bodies now yield the parameter; all other shapes are unchanged), but the new regression test does not cover this code path. The test in helper-reference.test.ts drives `preflightHelpers` through `refusals`, while this line lives in `flowRequirements`, which has its own separate parameter-extraction path — no test passes a generator-method body to `flowRequirements`, so reverting this line would still leave the suite green. Add a `flowRequirements` test with a `*gen(f)` method body asserting the helper integration is declared.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

// 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The regex change itself is correct (generator-method bodies now yield the parameter; all other shapes are unchanged), but the new regression test does not cover this code path. The test in helper-reference.test.ts drives preflightHelpers through refusals, while this line lives in flowRequirements, which has its own separate parameter-extraction path — no test passes a generator-method body to flowRequirements, so reverting this line would still leave the suite green. Add a flowRequirements test with a *gen(f) method body asserting the helper integration is declared.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/sdk/src/flow-requirements.ts, line 239:

<comment>The regex change itself is correct (generator-method bodies now yield the parameter; all other shapes are unchanged), but the new regression test does not cover this code path. The test in helper-reference.test.ts drives `preflightHelpers` through `refusals`, while this line lives in `flowRequirements`, which has its own separate parameter-extraction path — no test passes a generator-method body to `flowRequirements`, so reverting this line would still leave the suite green. Add a `flowRequirements` test with a `*gen(f)` method body asserting the helper integration is declared.</comment>

<file context>
@@ -236,7 +236,7 @@ function contextParameter(body: string): string | undefined {
   // 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];
 }
</file context>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid, and the best catch on this PR — fixed in 471e5ba4.

I confirmed it by mutation rather than by reading: reverting only the flow-requirements.ts regex left all 34 tests green, so that mirror was pinned by nothing. My test drove preflightHelpers through refusals; flowRequirements extracts the context parameter on its own path, so the two can drift silently.

Added a flowRequirements case per body shape — async method, plain method, generator method, $-prefixed parameter, plus 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.

Worth noting for the file as a whole: preflightHelpers and flowRequirements having separate parameter extraction is the same duplication that let the original string-scanning bug exist in two places. The detection itself is now shared in helper-reference.ts; the parameter regex is the last mirrored piece, and folding it in there too would be the tidier follow-up.

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) <noreply@anthropic.com>
@khaliqgant
khaliqgant merged commit 2a7f6a5 into main Sep 24, 2026
9 checks passed
@khaliqgant
khaliqgant deleted the chore/drop-surface-peer-override branch September 24, 2026 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant