Repository navigation
fix(electron): bind each contextBridge helper invocation to its own API - #6284
Conversation
Wrapper substitution keyed contextBridge findings only by the inner exposure call's range, so every helper invocation replaced the previous one while unioning its members: the last key survived with every invocation's members. Substituted members also listed only immediate object keys, dropping nested paths that direct syntax retains. Record substituted exposures per site and per world and key, and replace the site's syntax observation once after traversal. Invocations exposing one key at one site describe one API covering each alternative object; a traced invocation whose key, object or world stays unresolved or guarded keeps the site's own unresolved observation. Enumerate member paths through resolved objects with the direct syntax depth bound, which also retains the depth-limit unknown that substitution previously dropped. Fixes #5871 Fixes #5870 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kaoru0822-kitauji
added
bug
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. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
 [Medium impact] The changes affect the JavaScript and Electron analysis workflow and its output views. Fix the mixing of own-scope observations and helper invocations before merging. Findings
Fix with agent prompt### Issue 1
src/domain/javascript/electronWrappedCallPaths.ts:995-1008
The inner call's standalone evaluation is merged as another helper invocation. For example, `function expose(label, id, api) { contextBridge.exposeInIsolatedWorld(id, "fixed", api); } expose("label", 1001, { ping() {} });` first visits the inner call with unbound parameters. This records a dynamic `world_id` and `unknown_members: 1`. The actual invocation then merges into that record, so neither unknown is cleared. Previously, the later resolved invocation replaced that record.
Keep standalone observations separate from invocation contributions, and retain them only when needed for unresolved traced calls.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThe PR separates helper-provided APIs by key and adds bounded nested member paths.
Explicitly deferred by
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Exposure call site] --> B[Own-scope observation]
A --> C[Traced helper invocations]
B --> D[Shared contribution map]
C --> D
D --> E[Merge by world and API key]
E --> F[Replace syntax observation]
Reviews (1) · Last reviewed commit: "fix(electron): bind each contextBridge h..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const identity = bridgeExposureKey(finding); | ||
| const contribution = bridgeContributionKey(finding); | ||
| const prior = site.exposures.get(identity); | ||
| if (prior === undefined) | ||
| site.exposures.set(identity, { | ||
| value: finding, | ||
| contributions: new Set([contribution]), | ||
| }); | ||
| else if (!prior.contributions.has(contribution)) { | ||
| prior.contributions.add(contribution); | ||
| const merged = mergeBridgeExposures( | ||
| prior.value, | ||
| finding, | ||
| site.worldExpression, |
There was a problem hiding this comment.
The inner call's standalone evaluation is merged as another helper invocation. For example, function expose(label, id, api) { contextBridge.exposeInIsolatedWorld(id, "fixed", api); } expose("label", 1001, { ping() {} }); first visits the inner call with unbound parameters. This records a dynamic world_id and unknown_members: 1. The actual invocation then merges into that record, so neither unknown is cleared. Previously, the later resolved invocation replaced that record.
Keep standalone observations separate from invocation contributions, and retain them only when needed for unresolved traced calls.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/electronWrappedCallPaths.ts
Line: 995-1008
Comment:
**Resolved helper facts stay unknown**
The inner call's standalone evaluation is merged as another helper invocation. For example, `function expose(label, id, api) { contextBridge.exposeInIsolatedWorld(id, "fixed", api); } expose("label", 1001, { ping() {} });` first visits the inner call with unbound parameters. This records a dynamic `world_id` and `unknown_members: 1`. The actual invocation then merges into that record, so neither unknown is cleared. Previously, the later resolved invocation replaced that record.
Keep standalone observations separate from invocation contributions, and retain them only when needed for unresolved traced calls.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
What broke: Wrapper substitution looked up contextBridge findings only by the inner
exposeInMainWorldcall's range. Each helper invocation replaced the previous one and unioned its members, so the last key kept every invocation's members (#5871). Substituted members also listed only top-level keys, so nested paths were dropped (#5870).Fix: Substituted exposures are now stored per call site and per (world, key), with members enumerated recursively under the same depth-8 bound the direct syntax pass uses. The site's syntax observation is replaced once, after traversal. A traced invocation that stays unresolved or guarded keeps the syntax observation.
Risk: The change is limited to the wrapper-substitution branch for contextBridge exposures in
electronWrappedCallPaths.ts. Complete direct-syntax observations take the same path as before, and the IPC, call-path and graph projection code is unchanged.Summary
Fixes #5871
Fixes #5870
analyze-javascript-applicationmerged contextBridge exposures that go through a local helper. Callingexpose("one", {ping})andexpose("two", {pong})produced one API,two, with members["ping","pong"], even though the call pathsone.pingandtwo.pongwere retained. With this change, each helper invocation binds its own key and members. The API count, the per-key members and the exposure call-path entries now agree with the direct-call control. The same producer also dropped nested paths for helper-passed objects (["nested"]and nonested.ping); that is fixed here too.Problem and expected behavior
function expose(key, api) { contextBridge.exposeInMainWorld(key, api); }called twice with different literal keys.bridge()insrc/domain/javascript/electronWrappedCallPaths.tsbuilt the substituted finding from state shared by every invocation of the site, and only enumerated immediate keys of the symbolic object.Change and scope
BridgeSiteper exposure call site and keyed by (world, api key), which is the identity the direct syntax pass and the graph node use. After traversal,recordBridgeExposures()replaces the site's syntax observation in its original position.unknown_membersis the maximum. If the world ids differ,world_idbecomes dynamic. This avoids a duplicate graph identity and matches how a union-valued argument is already treated.bridgeMemberInventoryenumerates dotted paths through nested values that resolve to unconditional objects, records or namespaces. It uses the depth boundcontextBridgeMemberPathDepth = 8, now shared with the direct collector inelectronStaticAnalysisValues.ts, and shares the existing visit budget. This supersedes the old union with syntax members, which has been removed. It also keeps the depth-limit unknown that substitution used to drop.findover all bridge findings on each exposure is gone.expose(api, "key")orexposeInIsolatedWorldhelpers called with a numeric world id first. That limit predates this change, and those cases keep their unresolved syntax observation.api_key ?? api_key_expression. A dynamic-key observation whose expression text equals a resolved literal key at the same site would share a node.Contribution scope: ordinary fix within existing boundaries; maintainer scope decision not applicable.
Contract and boundary impact
collectElectronWrappedCallPaths(domain, wrapper substitution) insrc/domain/javascript/electronWrappedCallPaths.ts.context_bridge_apisfindings, graphcontext-bridge-apiobservations and summary counts now contain correct per-key values.resolution_basis: "wrapper-substitution"and static-inference Evidence. Direct observations are unchanged.docs/javascript-artifact-reconstruction.mdnow describes per-invocation exposure recording.Expected effects:
Evidence and regression coverage
tests/boundary/cli/electronWrappedCallPaths.test.tsadds "binds each contextBridge helper invocation's own key and members", run through both CLI and stdio MCP. One fixture covers repeated helper calls, a nested API object, a helper chain with key and API in different parameter positions, a direct call alongside helpers, and a traced invocation with an unresolved key. The test asserts 5 APIs, per-key members,exposed_api_members: 5, and that the exposure entries match.src/domain/javascript/electronWrappedCallPaths.test.tsaddscontextBridge helper invocations, 7 cases: site-literal object with a key parameter, repeated identical invocation, same key with alternative objects, differing isolated world ids, depth-8 parity with direct syntax, guarded invocation, and a site-literal key next to a guarded invocation. With the source fix stashed, 6 of the 7 fail on the base revision; the repeated-identical case already passed.b6007e0a5,node scripts/rea.mjs analyze-javascript-application <dir> --json, using the issue's extraction script):basis: direct-syntax,object-literal).three: [a, b, b.c].outer→inner, plus a directinnercall: 1 → 3 APIs.unknown_memberswas lost (0) → kept as 1 ononeonly.oneandtwo.one.api: [a, b].nested.deeper.pongand the other paths are listed.unknown_members: 1.unknown_members: 1in both. The depth unknown was previously dropped for the direct case.For evidence-bearing changes:
Validation performed
npm run test:local -- src/domain/javascript/electronWrappedCallPaths.test.ts: 179 passed. With the source fix stashed, the 6 new cases fail.npm run test:focused --allsrc/domain/javascript/*.test.ts, allsrc/application/javascript/*.test.ts, and the Electron/JS CLI, MCP, filesystem and acceptance suites (tests/boundary/cli/electronWrappedCallPaths.test.ts,tests/boundary/cli/electronCli.test.ts,tests/boundary/mcp/electronMcp.test.ts,tests/boundary/filesystem/javascript-applications/*(2 files),tests/boundary/javascript/workerOrderAudit.test.ts,tests/acceptance/applications/{applicationWorkflowCli,electronIpcConstantChannelCli,javascriptFactoryCompletion,electronIpcListenerCallsiteCli}.test.ts): 158 files, 4060 tests passed.npm run check:fast: passed (typecheck, lint).npm run format:check: passed.npm run docs:check: passed./usr/bin/time -l.Compatibility, safety, and release
Author readiness
Human owner: @kaoru0822-kitauji
Before merge
type(scope): outcome.🤖 Generated with Claude Code