Expose helper resource catalogs and honor shadowed context bindings - #516
Merged
Merged
Conversation
…talog f.gitlab carries comments and discussions; f.github carries issues, pull requests, reviews, refs, merge and close-pull-request. The catalog marked both `supported: true`, so the only way to learn the difference was to reach for `f.gitlab.issues` and read `undefined is not a function` — or to dump the writeback catalog before writing a line. A GitLab-sourced flow shelled out to `glab` for every read as a result. Parity is upstream work. What is fixed here is the silence: `supported` now distinguishes `'partial'` from full, the generator carries the resources each provider actually dispatches and the note that says what a partial omits, and every layer an author can reach the gap through refuses by name — `flows check` statically, the surface's property guard for a computed name, both worded by one function and neither touching the provider to refuse. Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
…ent on code it never read
The refusal for a member `f.gitlab` does not carry was reaching for surface
exports that the pinned, published surface does not ship, and was reading
text that is not a member access as one.
- The SDK no longer imports `unsupportedHelperMemberMessage` or
`UnsupportedHelperMemberError`. Both are unreleased, and this source is
installed against a published surface in the schema `validate` job, where a
missing named export fails the whole module at load — before preflight can
run. The wording is restated locally and pinned equal to the surface's in
test; the envelope remap matches `error.name`, which is also correct across
the realm boundary an authored flow file's own surface copy creates.
- Static inspection now admits every member the runtime guard still resolves.
The guard refuses only what the bound object lacks and is neither `then` nor
`toJSON`, so `f.gitlab.hasOwnProperty('comments')` returns `true` at run
time; `flows check` must not reject feature detection that works.
- A regular-expression literal is blanked with the other data literals, so
`/f.gitlab.issues/.test(line)` — which inspects text and reaches no helper —
no longer refuses. Ambiguous `/` resolves to "regex", which can only
withdraw a static refusal and leave the runtime guard to make it.
- A body that binds the context parameter's name again is left to the runtime
guard. Renaming a local callback parameter cannot decide whether a flow is
admitted, and an inner `f` need not be the flow context at all.
The last three can only withdraw refusals, never invent them; the runtime
guard remains the backstop for everything they decline to judge.
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
Relayflow: the adversarial review did not pass. This branch is not approved: the flow stopped here and did not mark it ready to merge. Review of PR #516Reviewed head: Changes requested. 1. [P1] Compile the SDK against its pinned published surfaceLocation: Removing the new named imports fixes the module-load failure, but the SDK still cannot compile against its declared dependency, I copied the current SDK source, package.json and tsconfig.json into Ship and pin compatible surface types, or explicitly normalize the old/new catalog shapes before reading the optional new fields. Add coverage that compiles with the published dependency rather than only the local surface. The now-green schema validation job runs Bun source tests and does not demonstrate this TypeScript build succeeds. 2. [P2] Do not refuse accesses on destructured locals or method parametersLocation: The rebinding detector handles simple declarations and function/arrow/catch parameters, but misses destructured declarations and method parameters. Both of these valid bodies return 42 without accessing a GitLab helper: (f) => { { const { f } = { f: { gitlab: { issues: 42 } } }; return f.gitlab.issues; } }
(f) => ({ read(f) { return f.gitlab.issues; } }).read({ gitlab: { issues: 42 } })With the GitLab mount fact satisfied, preflight rejects both as Scope and limitsReviewed all 16 changed files, the previous local review, and the PR discussion, inline comments and submitted reviews via paginated GitHub API requests. At the captured snapshot there is one CodeRabbit skipped-review notice, no inline comments, and no submitted reviews. The PR description still describes the original commit's scanner and tests; it is not evidence for the follow-up fixes. The follow-up covers the earlier inherited-member and simple regex reproductions with regression tests. The package compatibility problem persists at compilation, and shadowing remains incomplete as described above. No production source, generated file, test gate, or docs/evidence file was changed during this review. No live GitLab API verification is claimed. No PR comments were posted. Affected-package verification
Reproduction sourceThe probe executes harmless data-only bodies and calls preflight with a satisfied mount; no provider is contacted. import { preflightHelpers } from '/home/daytona/.relayflow-v2-supervisor/durable/repository/packages/sdk/src/helper-preflight.ts';
import { createHelpers } from '/home/daytona/.relayflow-v2-supervisor/durable/repository/packages/surface/dist/runtime.js';
const facts = {providers:{gitlab:{mount:true,mock:false}}};
const cases = {
destructured: '(f) => { { const { f } = { f: {gitlab:{issues:42}} }; return f.gitlab.issues; } }',
method: '(f) => ({ read(f) { return f.gitlab.issues; } }).read({gitlab:{issues:42}})',
namespacePrefix: '(f) => { f.gitlabExtra = {issues:42}; return f.gitlabExtra.issues; }',
regexAfterReturnNewline: '(f) => { return\n /f.gitlab.issues/.test("x"); }',
};
const helpers = createHelpers(() => {throw Error('unexpected dispatch')});
for (const [name,source] of Object.entries(cases)) {
try {const body = new Function(`return ${source}`)(); console.log(JSON.stringify({name, runtime:body({...helpers}), preflight:preflightHelpers({body},facts)}));} catch(e) { console.log(name,String(e));}
}Captured commandbun /tmp/gitlab-review-head/probe.tsExit code: 0. Captured stdout/stderr: Captured commandpackages/sdk/node_modules/.bin/tsc --noEmit -p /tmp/gitlab-review-head/clean-sdk/tsconfig.jsonExit code: 2. Captured stdout/stderr: Captured commandgit show origin/main:packages/sdk/src/helper-preflight.ts > /tmp/gitlab-review-head/clean-sdk/src/helper-preflight.ts
packages/sdk/node_modules/.bin/tsc --noEmit -p /tmp/gitlab-review-head/clean-sdk/tsconfig.jsonExit code: 0. Captured stdout/stderr (empty): Captured commandnpm test --prefix packages/surfaceExit code: 0. Captured stdout/stderr: Captured commandnpm run typecheck:regressions --prefix packages/surfaceExit code: 0. Captured stdout/stderr: Captured commandgh pr checks 516Exit code: 0. Captured stdout/stderr: Captured commandgh api --paginate repos/AgentWorkforce/flows/issues/516/commentsExit code: 0. Captured stdout/stderr: Captured commandgh api --paginate repos/AgentWorkforce/flows/pulls/516/commentsExit code: 0. Captured stdout/stderr: Captured commandgh api --paginate repos/AgentWorkforce/flows/pulls/516/reviewsExit code: 0. Captured stdout/stderr: Reproducing the dependency-isolated TypeScript checkThe temporary SDK keeps all other installed dependencies fixed and changes only surface resolution from the local symlink to the registry artifact. From the repository root, use a fresh temporary directory (the path below was used for the captured run): npm pack @relayflows/surface@2.0.22 --pack-destination /tmp/gitlab-review-head/registryThe source-copy and dependency setup used for the captured check: from pathlib import Path
import shutil, tarfile
repo=Path.cwd(); target=Path('/tmp/gitlab-review-head/clean-sdk'); target.mkdir(exist_ok=True)
shutil.copytree(repo/'packages/sdk/src',target/'src',dirs_exist_ok=True)
shutil.copy(repo/'packages/sdk/package.json',target/'package.json')
shutil.copy(repo/'packages/sdk/tsconfig.json',target/'tsconfig.json')
deps=target/'node_modules'; deps.mkdir(exist_ok=True)
for p in (repo/'packages/sdk/node_modules').iterdir():
if p.name=='@relayflows':
(deps/p.name).mkdir(exist_ok=True)
for child in p.iterdir():
if child.name!='surface': (deps/p.name/child.name).symlink_to(child.resolve())
else: (deps/p.name).symlink_to(p.resolve())
surface=deps/'@relayflows/surface'; surface.mkdir(exist_ok=True)
with tarfile.open('/tmp/gitlab-review-head/registry/relayflows-surface-2.0.22.tgz') as tar:
for m in tar.getmembers():
if m.name.startswith('package/'):
m.name=m.name[len('package/'):]; tar.extract(m,surface,filter='data')Registry pack command captured output (exit 0): Captured full SDK test commandnpm test --prefix packages/sdkExit code: 1. Full captured stdout/stderr: |
…nd destructured rebindings
…are-garden-22d463a6
Member
Supersession check — the upstream parity this PR anticipated has shippedThe PR body says: "Parity on
This is the promotion the generator's comment describes — not merely a larger resource count: the adapter implements the named handlers. The branch's premise ("Issue list/read/create and merge-request list/read/create are unavailable through f.gitlab") was true at adapter-core 0.5.26 and is false at 0.6.2. Two real findings from the adversarial review were also fixed on this branch meanwhile (head
If the coordinator/product call is that gitlab parity stands, this PR's feature is fully superseded and the right terminal state is close-with-evidence (the partial machinery would be dead code — no partial provider remains). If a mount-level check shows upstream gitlab writeback is NOT actually served, the branch is repaired and ready to proceed on its own merits. Holding the push pending that call. |
khaliqgant
changed the title
`typecheck:regressions` failed with TS2578: the `@ts-expect-error` on `f.gitlab.issues.list` was unused, because the helper namespace is structural and the call does typecheck. That is the design, not a gap. gitlab is `supported: "partial"` with resources comments and discussions, and the catalog refuses the rest at runtime with a named member, its resource and the available names -- `f.gitlab.issues is unavailable; available resources: comments, discussions.` -- covered across dot, bracket and aliased access in helper-support.test.ts and the SDK's helper-partial-support.test.ts. The directive claimed a type-level narrowing that was never implemented, so the file asserted something untrue and the compiler said so. The `mergeRequests` case stays: that member genuinely does not exist on the type, and removing its directive still raises TS2551. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
|
Pushed 4632825, which fixes the The remaining failure is a design contradiction, not a test problem, so I have not touched it.
- * Available resources: comments, discussions.
+ * Available resources: close-merge-request, comments, discussions, issues, merge, merge-requests, refs.The runtime failures in this job agree with the generator rather than the branch — the refusal raises So the branch's premise and its source of truth disagree, and only one can be right:
Regenerating to make CI green would silently choose (2) and revert this PR's whole point, so I stopped here. This needs the author's call. The other failing check, |
Adopt shipped GitLab writeback parity, keep the generated provider catalog reviewable, make helper-reference analysis scope-aware for destructured and method bindings, and refresh the hosted Surface byte review. Session-Id: 01a0d52d-03d5-7773-b760-d6b67d5d8235
khaliqgant
changed the title
Member
khaliqgant
approved these changes
Sep 24, 2026
khaliqgant
left a comment
khaliqgant
left a comment
Member
There was a problem hiding this comment.
Independent exact-head review of edd8c44. Guarded repair is based on current main e04c5b9; SDK typecheck and typecheck:tests pass, helper fanout suite 96/96 passes. Required checks are terminal green: linux 36058867396, packed 36058867409, validate 36058867400, guard 36058862811; Cubic and CodeRabbit statuses pass. No substantive blocker found in the focused helper-support/generator changes. Draft remains intentionally unchanged for owner/product completion.
khaliqgant
marked this pull request as ready for review
September 25, 2026 03:42
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
khaliqgant
marked this pull request as draft
September 25, 2026 03:45
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edd8c44a60
ℹ️ 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".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Session-Id: 01a0d6ad-a1bd-78b0-9284-d13e4db53e96
Session-Id: 01a0d6ad-a1bd-78b0-9284-d13e4db53e96
khaliqgant
requested changes
Sep 25, 2026
khaliqgant
left a comment
khaliqgant
left a comment
Member
There was a problem hiding this comment.
NO-GO at exact head 53a10fd7df74b1fd5cdea7d0283de561c0011104.
[P1] Keep root-flow parameter defaults visible — packages/sdk/src/helper-reference.ts:142
The new parameterHidden = shadowed || bindsParameter hides every parameter subtree on the root function because the root context parameter itself necessarily binds f. Later root parameter defaults may legitimately read the already-initialized context parameter, so (f, x = f.gitlab.issues) => x really uses GitLab. At this head the scanner reports no helper and preflight admits the flow without a GitLab mount, moving a provable failure to runtime. Nested functions whose own parameter list binds f should remain hidden; the root function is the exception.
Literal reproduction:
bun -e <probe importing packages/sdk/src/helper-preflight.ts and evaluating the three bodies below>
{"name":"rootDefault","kinds":[]}
{"name":"nestedDefault","kinds":["helper_provider.mount_required"]}
{"name":"nestedBound","kinds":[]}
The probe bodies were:
(f, x = f.gitlab.issues) => x
(f) => { function read(x = f.gitlab.issues) { var f = local; return f.gitlab; } }
(f) => { const g = (f, x = f.gitlab.issues) => x; return g; }The second and third results are correct; the first must contain helper_provider.mount_required.
Focused exact-head verification otherwise passed:
mise x node@22.23.2 -- npx vitest run tests/helper-reference.test.ts tests/helpers-fanout.test.ts
Test Files 2 passed (2)
Tests 125 passed (125)
mise x node@22.23.2 -- npm run typecheck --prefix packages/sdk
@relayflows/sdk@2.0.32 typecheck
tsc --noEmit && tsc -p tsconfig.type-tests.json
mise x node@22.23.2 -- npm run typecheck:tests --prefix packages/sdk
@relayflows/sdk@2.0.32 typecheck:tests
tsc -p tsconfig.tests.json
Main 979325af5cb59ff9b4892cacdeb58e1a10f700c3 is an ancestor of this head. At capture time validate and guard were green; linux-x64-artifact and packed-consumer were still in progress. Even if those finish green, this preflight miss blocks approval. Add the root-default regression and keep its parameter initializer traversal unshadowed before requesting a fresh exact-head review.
Session-Id: 01a0d6ad-a1bd-78b0-9284-d13e4db53e96
khaliqgant
approved these changes
Sep 25, 2026
khaliqgant
left a comment
khaliqgant
left a comment
Member
There was a problem hiding this comment.
APPROVE — independently reviewed exact head 28c46e45b64d658a59e6cec0d9fabb1bd81c4d2f.
Main 979325af5cb59ff9b4892cacdeb58e1a10f700c3 is an ancestor. The root parameter-default regression is fixed: (f, x = f.gitlab.issues) => x now reports helper_provider.mount_required; nested parameter/default and function-scoped var cases remain correctly scoped.
Literal focused verification:
npx vitest run tests/helper-reference.test.ts tests/helpers-fanout.test.ts
Test Files 2 passed (2)
Tests 126 passed (126)
npm run typecheck --prefix packages/sdk
tsc --noEmit && tsc -p tsconfig.type-tests.json
npm run typecheck:tests --prefix packages/sdk
tsc -p tsconfig.tests.json
Exact-head required checks:
guard pass
validate pass
packed-consumer pass
linux-x64-artifact pass
npm/pages skipped by workflow
All substantive review threads are outdated against this head and their findings are covered by the binding-position/default-expression, nested-var, and root-default fixes. Mergeability is CLEAN/MERGEABLE. Draft/readiness state remains unchanged; I did not mark ready, merge, or mutate repository files.
miyaontherelay
marked this pull request as ready for review
September 25, 2026 04:24
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 28c46e4. Configure here.
| hidden = true; | ||
| } else if (node.type === 'CatchClause' && isNode(node.param) && patternBinds(node.param, root)) { | ||
| hidden = true; | ||
| } |
There was a problem hiding this comment.
For-loop bindings skip shadowing
Low Severity
The new scope walk hides block, catch, and function bindings of the context name, but a for / for-in / for-of head that lexically binds that name still counts member access in the loop as an outer helper reference. Preflight can then demand a mount the flow never uses.
Reviewed by Cursor Bugbot for commit 28c46e4. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GitLab writeback parity has landed in the pinned adapter catalog, so this PR no
longer installs a partial-provider refusal. Instead it rebases the useful part
of the original work onto current main:
resources, including GitLab issues, merge requests, refs, merge, and close;and object/class method parameters named like the flow context do not cause
false mount refusals;
Surface runtime.
The obsolete partial-GitLab runtime/preflight machinery is removed by the
current-main merge. This also removes the
helper-preflight.tsreads that wereincompatible with the published
@relayflows/surface@2.0.22catalog type; theSDK now builds against its current pinned Surface package.
Deterministic regressions
resources; GitLab's exact current resource list is pinned;
register as outer helper references;
mount.
Verification
The explicit historical
@relayflows/surface@2.0.22probe emits none of thereported helper-preflight TS2367/TS2339/TS2345 diagnostics. A complete current
SDK build against that historical package is not expected to pass because main
now consumes additional, unrelated Surface APIs and pins 2.0.31.
Refs #507
Note
Medium Risk
Shadowing-aware helper detection changes which integrations preflight and
flowRequirementsdemand; incorrect scope logic could under- or over-require mounts, while catalog fields are additive metadata.Overview
This PR adds inspectable helper provider metadata and fixes false mount refusals when flow bodies shadow the context parameter.
Generated catalog:
generate-helpers.mjsnow emits a sortedresourceslist per provider (from the pinned adapter writeback catalog) next tosupported, including GitLab’s full current set (issues,merge-requests,refs,merge,close-merge-request, etc.). Unsupported providers get emptyresources. Docs and a snapshot test lock sorting, empty unsupported lists, and GitLab parity. The hosted extension sandbox SHA256 pin forhelpers/providers.jsis updated to match the regenerated file.Preflight / requirements:
helperNamespacesUsedreplaces a blind AST walk withwalkReferences, which tracks whenf(or the flow root name) is shadowed by destructuring, nested method parameters, lexical blocks,catchparams, or function-scopedvar—so innerf.gitlabaccess no longer demands a GitLab mount the outer flow never uses. Real outer references after a shadowing block still require mounts. New tests cover those shapes for both preflight andflowRequirements.Reviewed by Cursor Bugbot for commit 28c46e4. Bugbot is set up for automated code reviews on this repo. Configure here.