Repository navigation
Fix saved app facet exec and partial update semantics - #165
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughRefactors saved-app backend execution to compile one-off code into a throwaway Dynamic Worker with an explicit RPC bridge to saved app facets. Implements partial update semantics for saved apps (omitted fields preserve existing values; Changes
Sequence DiagramsequenceDiagram
participant Client
participant AppRunner as AppRunner (RPC)
participant Compiler as CodeCompiler
participant ExecWorker as SavedAppExecWorker (Throwaway)
participant AppFacet as SavedApp Facet
Client->>AppRunner: codemode.app_server_exec(code, params)
AppRunner->>Compiler: compile worker module from `code`
Compiler-->>AppRunner: worker entrypoint module
AppRunner->>ExecWorker: load & run(params)
ExecWorker->>ExecWorker: evaluate provided function body
ExecWorker->>AppRunner: app.call(methodName, args) (RPC)
AppRunner->>AppRunner: callFacetRpc(appId, facetName, methodName, args)
AppRunner->>AppFacet: invoke __kody_invokeUserMethod(methodName, args)
AppFacet-->>AppRunner: method result
AppRunner-->>ExecWorker: return result to snippet
ExecWorker-->>Client: snippet result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
April 14, 2026 01:00
Contributor
|
🔎 Preview deployed: https://kody-pr-165.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/app-runner.ts`:
- Around line 158-170: The injected worker module created by
createSavedAppExecWorkerModule (class ${savedAppExecEntrypointName}) currently
allows injected code to access this.env.APP directly and call any facet methods;
instead, prevent direct access to this.env.APP by only exposing a local app
helper whose call method routes to a new, dedicated RPC on the generated facet
module (e.g., facet.invokeUserMethod) that performs validation: accept only
allowed user-exported method names and/or a whitelist and explicitly reject
reserved names/prefixes like fetch and __kody_*; update
createSavedAppExecWorkerModule and the corresponding facet RPC implementation
(also apply same change to the other injection site around the existing
app_server_exec logic) so app.call(...) invokes the wrapper RPC which enforces
the allowed-methods policy before dispatching to the real facet.
In `@packages/worker/src/mcp/capabilities/apps/ui-save-app.ts`:
- Around line 264-279: The updates object currently always sets parameters from
serializedParameters (which falls back to existingApp.parameters), causing
client-only updates to overwrite parameters; change the logic so
serializedParameters does NOT default to existingApp.parameters and ensure
updates.parameters is only set when args.parameters !== undefined (i.e., set
parameters to serializedParameters when args.parameters is provided, otherwise
leave parameters undefined/omit the property) so updateUiArtifact(...) can treat
undefined as “leave unchanged”; adjust the code around serializedParameters,
args.parameters, existingApp.parameters and the updates object accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16bdad33-d790-460a-80c1-0115a872df6f
📒 Files selected for processing (6)
docs/use/saved-app-backends.mddocs/use/skills-and-apps.mdpackages/worker/src/mcp/app-runner.tspackages/worker/src/mcp/capabilities/apps/app-server-exec.tspackages/worker/src/mcp/capabilities/apps/ui-save-app.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Exec worker
paramsdefaults to undefined instead of{}- Guarded the exec worker invocation by defaulting missing params to an empty object to preserve documented behavior.
- ✅ Fixed: Unused
getFacetRpcStubmethod is dead code- Removed the unused
getFacetRpcStubmethod and its RPC type entry since no callers exist.
- Removed the unused
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 61a71de. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/mcp/app-runner.ts (1)
540-541: Consider documenting the validation-onlygetFacetStubcall.Line 540 calls
getFacetStubbut discards the result. This appears intentional—to validate the facet exists and configuration is valid before loading the exec worker. A brief comment would clarify this is for early validation rather than an oversight.📝 Suggested clarification
async execServer(input: { appId: string facetName?: string | null code: string params?: Record<string, unknown> }) { const facetName = buildFacetName(input.facetName) + // Validate facet exists and config is valid before loading exec worker await this.getFacetStub(facetName) const config = await this.readConfig(this.ctx.id.toString())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/app-runner.ts` around lines 540 - 541, The call to this.getFacetStub(facetName) is intentionally made for validation only (its return value is discarded) before loading the exec worker; add a concise inline comment above or next to the call explaining that getFacetStub is invoked solely to validate the facet exists and its configuration before proceeding (so the reader knows the discarded result is intentional), referencing the call site this.getFacetStub(facetName) and the subsequent readConfig(this.ctx.id.toString()) context to make the intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/worker/src/mcp/app-runner.ts`:
- Around line 540-541: The call to this.getFacetStub(facetName) is intentionally
made for validation only (its return value is discarded) before loading the exec
worker; add a concise inline comment above or next to the call explaining that
getFacetStub is invoked solely to validate the facet exists and its
configuration before proceeding (so the reader knows the discarded result is
intentional), referencing the call site this.getFacetStub(facetName) and the
subsequent readConfig(this.ctx.id.toString()) context to make the intent clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec964701-a4ab-4e9c-b708-1a66d63e9edf
📒 Files selected for processing (5)
docs/use/saved-app-backends.mdpackages/worker/src/mcp/app-runner.tspackages/worker/src/mcp/capabilities/apps/app-server-exec.tspackages/worker/src/mcp/capabilities/apps/ui-save-app.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/worker/src/mcp/capabilities/apps/app-server-exec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts
This was referenced Apr 14, 2026
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
2 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
app_server_execoff in-facet string eval and into a throwaway Dynamic Worker loaded viaAPP_LOADERui_save_appupdates unless the caller explicitly changes themDesign note
app_server_execno longer attempts to evaluate arbitrary source inside the running saved-app facet. The supervisor wraps the snippet in a synthetic WorkerEntrypoint module, loads it through the Dynamic Worker Loader API, and exposes an explicitapp.call(methodName, ...args)bridge. That bridge now routes through a dedicated facet RPC wrapper that rejects reserved names such asfetchand__kody_*before dispatching to user-defined methods on the saved app's exportedAppclass. For raw storage inspection, users should useapp_storage_export.Testing
packages/worker/src/mcp/mcp-server.mcp-e2e.test.tsapp_server_exectrivial execution, explicit RPC calls, and reserved-method rejectionui_save_apppreserve / clear / rotate backend semantics across fresh MCP sessionsparametersdo not overwrite existing saved parameter definitionsSummary by CodeRabbit
New Features
Documentation
Tests