[APPS][Connections Part 3] Resolve same-module backend connection IDs#340
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8000ecd7eb
ℹ️ 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".
| const init = bindings.consts.get(name); | ||
| if (!init) { | ||
| fail( | ||
| `Unsupported connectionId expression in ${filePath}: identifier '${name}' is not a top-level same-file const binding.`, | ||
| ); |
There was a problem hiding this comment.
Respect local shadowing before resolving constants
When a backend function declares a local CONNECTION_ID that shadows a top-level const, this lookup still resolves the identifier from the module-wide top-level map because the walker visits calls in nested scopes without checking local bindings. For example, const CONNECTION_ID = 'prod'; function run() { const CONNECTION_ID = getId(); request({ connectionId: CONNECTION_ID }); } would emit prod instead of failing closed for the dynamic local value, producing an incorrect connection manifest.
Useful? React with 👍 / 👎.
8000ecd to
b3ebcdc
Compare
b9320d3 to
3e1b24d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3ebcdc55e
ℹ️ 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".
| ); | ||
| } | ||
|
|
||
| const init = bindings.consts.get(name); |
There was a problem hiding this comment.
Check lexical scope before resolving consts
When a backend function has a local binding that shadows a top-level connection constant, this lookup still resolves the identifier from the module-level consts map because the AST walk does not track lexical scope. For example, with const CONNECTION_ID = 'prod' at the top level and function run() { const CONNECTION_ID = getConnectionId(); request({ connectionId: CONNECTION_ID }); }, the manifest records 'prod' even though the call uses the inner dynamic value; the fail-closed behavior should reject the scoped binding instead of emitting a wrong connection ID.
Useful? React with 👍 / 👎.
3e1b24d to
97a160d
Compare
b3ebcdc to
a367b98
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a367b98b6f
ℹ️ 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".
| if (node.type === 'Identifier') { | ||
| return resolveConnectionIdIdentifier(node.name, bindings, filePath, resolutionStack); |
There was a problem hiding this comment.
Respect shadowed connectionId identifiers
When a backend call is inside a nested scope that declares the same name as a top-level const, this resolves the connectionId from the module-level map rather than the binding that is actually in scope. For example, with const CONNECTION_ID = 'prod' at the top level and function run() { const CONNECTION_ID = 'dev'; request({ connectionId: CONNECTION_ID }); }, the manifest records prod even though the runtime call uses dev, so the generated backend-function metadata can omit the connection that is actually required.
Useful? React with 👍 / 👎.
| if (objectInit.type !== 'ObjectExpression') { | ||
| fail( | ||
| `Unsupported connectionId expression in ${filePath}: object '${objectName}' must be initialized to an object literal.`, | ||
| ); | ||
| } | ||
|
|
||
| return resolveObjectMemberValue(objectInit, propertyName, bindings, filePath, resolutionStack); |
There was a problem hiding this comment.
Fail closed when const connection maps are mutated
This treats any top-level const object literal as immutable, but const only freezes the binding, not its properties. In modules that update the map before or around the backend call, e.g. const CONNECTIONS = { HTTP: 'old' }; CONNECTIONS.HTTP = 'new'; request({ connectionId: CONNECTIONS.HTTP });, the extractor records old while runtime uses new; the existing checks fail closed for spreads/computed keys because they can hide members, and property assignments can hide them in the same way.
Useful? React with 👍 / 👎.
a367b98 to
a735063
Compare
97a160d to
9bcbfc1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a735063a8d
ℹ️ 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".
| if (node.type === 'Identifier') { | ||
| return resolveConnectionIdIdentifier(node.name, bindings, filePath, resolutionStack); |
There was a problem hiding this comment.
Avoid resolving shadowed IDs from top-level consts
When a call is inside an inner scope that declares a local CONNECTION_ID, walk still visits that call but collectSameModuleBindings only recorded top-level bindings; this branch then resolves the identifier by name from the top-level map without checking lexical shadowing. In the inspected backend extraction flow, const CONNECTION_ID = 'prod'; function run() { const CONNECTION_ID = 'dev'; request({ connectionId: CONNECTION_ID }); } records prod even though runtime sends dev, corrupting the manifest; please fail closed on shadowed identifiers or track scopes before resolving.
Useful? React with 👍 / 👎.
9bcbfc1 to
a0cea42
Compare
a735063 to
397f9ba
Compare
a0cea42 to
806c29a
Compare
1b589d7 to
e56eae5
Compare
61615b2 to
5e1d743
Compare
e56eae5 to
e8fff5b
Compare
5e1d743 to
c5c7d75
Compare
e8fff5b to
e142b33
Compare
e142b33 to
883e8e4
Compare
0affdd3 to
a013be2
Compare
883e8e4 to
638ffe0
Compare
a013be2 to
5c281d2
Compare
638ffe0 to
705b4da
Compare
5c281d2 to
c3c12a6
Compare
705b4da to
c3a5e2c
Compare

Motivation
This is part 3 of the connection ID manifest stack. Inline string literals are too narrow for realistic backend functions, where authors commonly name connection IDs or group them in a local object.
Changes
Extends same-file extraction to resolve static values declared in the backend module. The extractor now supports top-level
conststring identifiers, const-to-const chains, static template literals without interpolations, and one-level object member reads from same-file const object literals.Examples now supported:
The PR preserves fail-closed behavior for mutable bindings, unresolved identifiers, imported values, dynamic templates, computed/spread object forms, function calls, env reads, and nested member chains. Imported module traversal remains deferred to part 4.
QA Instructions
NODE_OPTIONS=--experimental-vm-modules ~/.yarn/switch/bin/yarn workspace @dd/tests jest --config jest.config.ts ../plugins/apps/src/backend/discovery.test.ts ../plugins/apps/src/backend/extract-connection-ids.test.ts ../plugins/apps/src/index.test.ts ../plugins/apps/src/vite/index.test.ts ../plugins/apps/src/vite/dev-server.test.ts ../plugins/apps/src/upload.test.ts --runInBand.~/.yarn/switch/bin/yarn workspace @dd/apps-plugin typecheck.~/.yarn/switch/bin/yarn workspace @dd/tests typecheck.Blast Radius
This affects static analysis inside a single backend entry module. It does not load or resolve imported modules, so behavior remains bounded to files already being transformed by the apps plugin.
Documentation