Skip to content

Performance: package app loading, MCP execute isolate reuse, and hot-path caching - #605

Merged
kentcdodds merged 7 commits into
mainfrom
cursor/perf-optimizations-ebb7
Jul 4, 2026
Merged

kentcdodds merged 7 commits into
mainfrom
cursor/perf-optimizations-ebb7

Conversation

@kentcdodds

@kentcdodds kentcdodds commented Jul 4, 2026 •

Copy link
Copy Markdown
Owner

Performance optimizations across package apps, MCP execute, and hot paths

Package apps were slow to load. An audit of the dynamic-worker pipeline found that the runtime bundle lookup used a different KV key than the publish pipeline writes, so every app load after a publish re-bundled from scratch, plus several other hot-path inefficiencies. This PR fixes the critical path and a set of related wins across the board.

Package app loading (the critical path)

  • Fix bundle artifact identity mismatch: buildPackageAppWorker read KV with a JSON-array cache key while publish-time rebuildPublishedPackageArtifacts writes under the canonical bundle-artifact:v1:... key with a D1 index row. Runtime now uses loadPublishedBundleArtifactByIdentity (kind app, artifactName null) — the same identity the publish pipeline persists — and persists rebuilds via persistPublishedBundleArtifact. Publish-time bundling work is no longer wasted.
  • Manifest-first fast path: on an artifact hit, the handler loads only the package manifest instead of every published source file; full source loads happen only on the rebuild path (via a loadSourceFiles callback).
  • Parallel host setup: package manifest load and caller-context creation now run under Promise.all.
  • Worker isolate reuse: the built worker options (bundle + hydrated module graph) are memoized in an isolate-level PromiseLruCache keyed by userId, packageId, kodyId, sourceId, publishedCommit, baseUrl, and caller email/displayName (only active for published commits). Each request then acquires a fresh stub via APP_LOADER.get(stableContentHashedId, () => cachedOptions), so workerd reuses the warm isolate while the stub stays bound to the current request. (Stubs themselves are request-scoped in workerd and cannot be cached — caching them 500s on the second request, which manual testing caught.)
  • Deduped D1 reads: loadPublishedEntitySource / loadPublishedEntityManifest accept a pre-resolved source row, removing a duplicate getEntitySourceById per source load. The persist fast path enforces the same ownership check as the DB-lookup branch.

SSR hot path (follow-up after #601 merged)

  • Router + env memoization: handleRequest built the full Remix router (~40 handler factories) and re-ran env schema validation on every request. Both are now memoized per Env object identity (stable per isolate); handlers already close over env, and the router keeps no per-request state.
  • Parallel admin queries: loadAdminUsersData issues the COUNT(*) and page SELECT concurrently.
  • Request-scoped community detail load: one /community/:id SSR response invoked loadCommunityDetailData twice (HTML handler for the loaderData embed + frame renderer during streaming). It is now memoized per Request.
  • Throttled session revalidation: the client refetched /session (2 D1 queries) on every SPA navigation. Navigation-triggered refreshes are now throttled to 30s; hydration and explicit refreshes (login/logout/profile updates) always go through.

MCP execute

  • Dynamic worker isolate reuse for bundled modules: stable content-hashed worker IDs were previously only allowed when the module map contained just executor.js, so every bundled execute (any module with imports) got a fresh isolate via crypto.randomUUID(). The hash covers the full module map, gateway props (including userId), timeout, and compatibility settings; non-deterministic module shapes still fall back to random IDs. Cache key version bumped to 2.
  • Registry pass-through: the capability registry loaded for logging in the execute tool is now threaded into runModuleWithRegistry instead of being rebuilt inside buildCodemodeFns.
  • Single-pass result serialization: limitExecutionResultValue no longer JSON-stringifies large results twice.

Module graph

  • Dynamic kody: import hydration resolves unique specifiers per pass with Promise.all instead of sequentially.
  • Direct kody dependency resolution parallelized with deterministic output ordering preserved.
  • The kody:runtime virtual module source is memoized (it is parameter-free).

Remote connectors and capability registry

  • New isolate-level snapshot cache (30s TTL) shared by registry synthesis, MCP DO init, and connector clients — a cold MCP call previously hit each connector session DO 2-3 times. Failed connector RPCs evict the cached snapshot so a disconnect is not masked for the TTL.
  • The built capability registry is memoized per user + connector snapshot identity (connector refs, connectedAt, tool names). Users without connectors keep the static registry fast path.

Secrets and values

  • Derived CryptoKeys for the server-wide secret-store key are cached at module level (previously SHA-256 + importKey on every encrypt/decrypt).
  • Secret placeholder resolution (capability inputs and fetch gateway) deduplicates and parallelizes lookups; resolveSecret resolves scopes in parallel via Promise.allSettled while preserving session → app → user precedence (a corrupted lower-precedence entry cannot fail a resolved higher-precedence secret).
  • listValues replaces per-bucket N+1 metadata queries with a single JOIN query (runs on every MCP search for signed-in users).

Multi-user isolation

Every new cache is keyed by userId (worker options additionally by full caller identity; connector snapshots by user-scoped session key; registry by userId + connector identity). Unit tests assert users never share cache entries. The router/env memoization is user-agnostic by design (it caches only env parsing and handler wiring, never request or user state).

Measured impact (local dev, tools/perf-smoke harness)

Publish a small package app via MCP execute, then load it over HTTP six times (3 runs each): first load improved from 51–107ms on main to 32–57ms on this branch, and warm loads from ~20–32ms to ~13–19ms. Local dev understates the win — production KV/D1 latency makes the eliminated re-bundle and duplicate reads count for more.

Coordination with other open PRs

Rebased onto main after #604 (integrations) and #601 (SSR) merged; no conflicts. The SSR follow-up section above implements the low-risk wins from a perf audit of the merged SSR pipeline. Larger deferred items (community catalog SQL pagination/search, trimming hydration payloads for secrets/READMEs, fragment caching for frames) are candidates for future PRs.

System recap — extends existing primitives (medium risk)

Mode: recap · Base: main @ d5a6147 · Head: 2af044f

Classification: extends — no new primitives; caching and lookup-identity behavior of several runtime primitives changed. No schema changes.

Primitives touched

Primitive Group Impact
app-ui runtime extends — memoized router/env, request-scoped detail loads, throttled session refresh
package-apps runtime extends — canonical artifact lookup, manifest fast path, isolate reuse via stable worker ids
package-runtime runtime extends — parallel hydration/dependency resolution, memoized runtime module
codemode-execute runtime extends — stable worker IDs for bundled modules, registry pass-through
capability-registry runtime extends — per-user registry memoization (30s TTL)
remote-connectors runtime extends — shared 30s snapshot cache with failure eviction
secrets storage composes — CryptoKey cache, parallel placeholder/scope resolution
values storage composes — single JOIN replaces per-bucket N+1 queries
bundle-artifacts-kv storage composes — runtime reads/writes now use the existing canonical keys
d1-app-db storage composes — fewer round trips; no schema change

System map

flowchart LR
	appUi["app-ui"]:::extended
	packageApps["package-apps"]:::extended
	packageRuntime["package-runtime"]:::extended
	codemodeExecute["codemode-execute"]:::extended
	capabilityRegistry["capability-registry"]:::extended
	remoteConnectors["remote-connectors"]:::extended
	secrets["secrets"]:::touched
	values["values"]:::touched
	bundleArtifactsKv["bundle-artifacts-kv"]:::touched
	d1AppDb["d1-app-db"]:::touched
	mcpServer["mcp-server"]:::untouched
	appUi --> packageApps --> packageRuntime --> bundleArtifactsKv
	packageRuntime --> d1AppDb
	appUi --> d1AppDb
	mcpServer --> codemodeExecute --> capabilityRegistry --> remoteConnectors
	codemodeExecute --> secrets
	mcpServer --> values
	classDef touched fill:#1a7f37,color:#fff
	classDef extended fill:#9a6700,color:#fff
	classDef added fill:#cf222e,color:#fff
	classDef untouched fill:#57606a,color:#fff
Loading

Change flow

sequenceDiagram
	participant B as Browser
	participant H as package-app handler
	participant D1 as D1
	participant KV as bundle-artifacts KV
	participant L as APP_LOADER
	Note over H: before — full source load + rebundle + new isolate per request
	B->>H: GET /@user/packages/:kodyId
	H->>D1: manifest row (parallel with caller ctx)
	H->>D1: artifact identity row
	H->>KV: canonical bundle artifact (hit)
	H->>L: get(stable worker id) with cached options — isolate reuse
	L-->>B: response
Loading

Invariants

per-user-isolation: every new cache (worker options/ids, connector snapshots, registry, source rows) is keyed by userId; unit tests assert no cross-user sharing. Stable dynamic worker IDs include userId in the hashed material. Router/env memoization holds no user or request state.

Open in Web Open in Cursor 

Summary by CodeRabbit

  • New Features
    • Added remote-connector snapshot caching and capability-registry memoization, including support for using a prebuilt capability registry.
    • Improved package app runtime loading by loading manifests up-front and reusing worker setup/options.
  • Bug Fixes
    • Deduplicated secret placeholder resolution and resolved secrets across scopes more robustly.
    • Improved limited-output truncation formatting (including more accurate display handling).
  • Performance
    • Parallelized multiple lookups (secrets, snapshots, module/package dependency resolution, admin-user totals).
    • Memoized env parsing, SSR community details, and reduced redundant session refreshes.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR shifts package app loading to manifests, adds cached worker and registry reuse, tightens executor hashing and truncation output, parallelizes secret resolution, and changes value metadata listing to a bulk bucket query.

Changes

Package App Manifest Loading and Worker Build Caching

Layer / File(s) Summary
Host setup loads package manifest
packages/worker/src/app/handlers/package-app.ts, packages/worker/src/app/handlers/package-app.node.test.ts
Host setup now loads the package manifest in parallel with caller context creation and derives worker setup data from it, with tests updated to mock manifest loading.
Published source loaders accept source rows
packages/worker/src/package-registry/source.ts, packages/worker/src/repo/published-source.ts
Published source and manifest loaders now accept an optional source row and validate it before use.
Worker build caching and published artifact reuse
packages/worker/src/package-runtime/package-app.ts, packages/worker/src/package-runtime/package-app.node.test.ts, packages/worker/src/package-runtime/published-bundle-artifacts.node.test.ts
Package app worker building now caches worker options, reuses warm isolates when possible, and loads or rebuilds published app bundles by identity.
Module graph memoization and parallel resolution
packages/worker/src/package-runtime/module-graph.ts, packages/worker/src/package-runtime/module-graph.node.test.ts
Runtime module source is memoized and dependency resolution/hydration now runs concurrently.

Estimated code review effort: 4 (Complex) | ~75 minutes

Capability Registry and Remote Connector Snapshot Caching

Layer / File(s) Summary
Remote connector snapshot cache
packages/worker/src/remote-connector/snapshot-cache.ts, packages/worker/src/remote-connector/snapshot-cache.node.test.ts
Adds a TTL-based LRU cache for remote connector snapshots keyed by user and connector identity, with invalidation helpers and coverage for reuse, disconnect handling, and scoping.
Client wiring to cache
packages/worker/src/remote-connector/client.ts
The MCP client now reads snapshots through the shared cache helper and invalidates cache entries after RPC failures.
Capability registry caching using snapshots
packages/worker/src/mcp/capabilities/registry.ts, packages/worker/src/mcp/capabilities/registry.node.test.ts, packages/worker/src/mcp/capabilities/remote-connector/index.ts
Capability registry construction now caches built registries and synthesizes remote domains from fetched snapshots.
Prebuilt capability registry through codemode execution
packages/worker/src/mcp/run-codemode-registry.ts, packages/worker/src/mcp/run-codemode-registry.node.test.ts, packages/worker/src/mcp/tools/execute.ts, packages/worker/src/mcp/tools/execute.node.test.ts
Codemode execution entrypoints accept an optional prebuilt capability registry and pass it through to module execution.

Estimated code review effort: 4 (Complex) | ~60 minutes

Executor Worker ID Determinism and Output Truncation

Layer / File(s) Summary
Deterministic module hashing
packages/worker/src/mcp/executor.ts, packages/worker/src/mcp/executor.node.test.ts
Worker id reuse now requires deterministically hashable modules, and tests cover the revised reuse conditions.
Truncation output with displayText
packages/worker/src/mcp/executor.ts, packages/worker/src/mcp/executor.node.test.ts
Execution result limiting now returns display text and uses separate truncation paths for strings and non-strings.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Secrets Caching and Parallel Resolution

Layer / File(s) Summary
Cached CryptoKey derivation
packages/worker/src/mcp/secrets/crypto.ts, packages/worker/src/mcp/secrets/crypto.node.test.ts
Encryption key derivation is cached per purpose and secret, and failed derivations are evicted.
Deduplicated and parallel secret placeholder resolution
packages/worker/src/mcp/secrets/capability-inputs.ts, packages/worker/src/mcp/secrets/capability-inputs.node.test.ts, packages/worker/src/mcp/fetch-gateway.ts
Duplicate secret placeholders are deduplicated and resolved concurrently, and fetch-gateway resolves secret placeholders in parallel.
Parallel scope resolution
packages/worker/src/mcp/secrets/service.ts, packages/worker/src/mcp/secrets/service.node.test.ts
Secret lookup now resolves scopes concurrently and returns the first match by precedence order.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Bulk Value Metadata Retrieval

Layer / File(s) Summary
Bulk metadata query
packages/worker/src/mcp/values/repo.ts, packages/worker/src/mcp/values/service.ts, packages/worker/src/mcp/values/service.node.test.ts
Value listing now fetches metadata across all buckets with a single bulk query instead of one query per bucket.

Estimated code review effort: 2 (Simple) | ~15 minutes

Sequence Diagram(s)

sequenceDiagram
  participant package_app_ts as package-app.ts
  participant loadPackageManifestBySourceId as loadPackageManifestBySourceId
  participant WorkerCache as WorkerOptionsCache
  participant PublishedArtifacts as PublishedBundleArtifacts
  participant APP_LOADER as APP_LOADER

  package_app_ts->>loadPackageManifestBySourceId: loadPackageManifestBySourceId(sourceId)
  loadPackageManifestBySourceId-->>package_app_ts: source + manifest
  package_app_ts->>WorkerCache: getOrCreate(cacheKey)
  WorkerCache->>PublishedArtifacts: loadPublishedBundleArtifactByIdentity
  alt artifact exists
    PublishedArtifacts-->>WorkerCache: bundle
  else missing
    WorkerCache->>PublishedArtifacts: build and persistPublishedBundleArtifact
  end
  WorkerCache-->>package_app_ts: workerOptions
  package_app_ts->>APP_LOADER: APP_LOADER.get(workerId, workerOptions)
Loading
sequenceDiagram
  participant getCapabilityRegistryForContext as getCapabilityRegistryForContext
  participant SnapshotCache as getCachedRemoteConnectorSnapshot
  participant RegistryCache as capabilityRegistryCache
  participant synthesizeRemoteToolDomain as synthesizeRemoteToolDomain
  participant runBundledModuleWithRegistry as runBundledModuleWithRegistry

  runBundledModuleWithRegistry->>getCapabilityRegistryForContext: getCapabilityRegistryForContext (if no prebuilt registry)
  getCapabilityRegistryForContext->>SnapshotCache: fetch snapshot per connector
  SnapshotCache-->>getCapabilityRegistryForContext: RemoteConnectorSnapshot
  getCapabilityRegistryForContext->>RegistryCache: getOrCreate(cacheKey)
  RegistryCache->>synthesizeRemoteToolDomain: synthesizeRemoteToolDomain(ref, snapshot)
  synthesizeRemoteToolDomain-->>RegistryCache: domain
  RegistryCache-->>getCapabilityRegistryForContext: BuiltCapabilityRegistry
  getCapabilityRegistryForContext-->>runBundledModuleWithRegistry: capabilityRegistry
Loading

Possibly related PRs

  • kentcdodds/kody#155: Both PRs modify remote-connector and capability-registry wiring around snapshot-aware tool synthesis.
  • kentcdodds/kody#381: Both PRs change packages/worker/src/mcp/executor.ts output limiting and formatting behavior.
  • kentcdodds/kody#541: Both PRs touch executor dynamic worker-id generation and related reuse tests.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the PR’s main themes: performance work in package app loading, MCP execute isolate reuse, and hot-path caching.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/perf-optimizations-ebb7

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.

@cursor
cursor Bot force-pushed the cursor/perf-optimizations-ebb7 branch from 7b23edc to 6d8a223 Compare July 4, 2026 16:31
@kentcdodds
kentcdodds marked this pull request as ready for review July 4, 2026 16:39
@github-actions

github-actions Bot commented Jul 4, 2026 •

Copy link
Copy Markdown
Contributor

🔎 Preview deployed: https://kody-pr-605.kentcdodds.workers.dev

Worker: kody-pr-605
D1: kody-pr-605-db
KV: kody-pr-605-oauth-kv

Mocks:

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/worker/src/package-registry/source.ts (1)

143-179: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Thread the resolved source through loadPackageSourceBySourceId package-app.ts reads the same row again in loadSourceFiles, so a republish between the manifest load and file load can pair one snapshot’s identity with another snapshot’s files. Accept an optional source here and pass packageManifest.source through to keep both reads on the same snapshot.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/package-registry/source.ts` around lines 143 - 179,
`loadPackageSourceBySourceId` re-reads the source row, which can mix snapshots
between manifest and file loading. Update this function to accept an optional
resolved `source` and reuse it when provided instead of always calling
`resolvePackageSourceRow`. Then thread `packageManifest.source` from
`loadSourceFiles` in `package-app.ts` into `loadPackageSourceBySourceId` so the
manifest and file reads stay on the same snapshot.
🧹 Nitpick comments (4)
packages/worker/src/mcp/secrets/service.node.test.ts (1)

2-2: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a regression test for scope-precedence when a lower-precedence scope fails to decrypt.

Good coverage for the happy path, but the new Promise.all-based resolveSecret (see comment in service.ts) can now fail the whole call if a lower-precedence scope's entry is corrupted, even when a higher-precedence scope resolves successfully. A test seeding session with a valid secret and user/app with an entry that fails to decrypt (e.g. malformed encrypted_value) would catch this regression.

Also applies to: 218-274

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/secrets/service.node.test.ts` at line 2, Add a
regression test around resolveSecret in service.node.test.ts that seeds a
higher-precedence scope (session) with a valid secret and a lower-precedence
scope (user or app) with a corrupted entry that fails decryption; then assert
the call still returns the session value instead of rejecting. Use the existing
listSecrets, resolveSecret, and saveSecret test helpers to locate the flow, and
make sure the malformed encrypted_value only affects the lower-precedence scope
so the test covers scope-precedence behavior under Promise.all.
packages/worker/src/mcp/run-codemode-registry.ts (1)

335-347: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Prebuilt capabilityRegistry is trusted without validating it matches callerContext's user.

buildCodemodeFns uses options.capabilityRegistry.capabilityMap directly whenever supplied, with no check that the registry was built for the same userId/connectors as callerContext. Current call sites (e.g., execute.ts) build the registry from the same callerContext right before passing it in, so there's no active leak today. But since getCapabilityRegistryForContext's cache key is userId-scoped specifically to prevent cross-user capability/connector leakage, a future caller that accidentally threads a registry built for one user into execution for a different user's callerContext would silently bypass that isolation.

Consider adding a lightweight assertion (e.g., compare callerContext.user?.userId against a userId tag stored on BuiltCapabilityRegistry) to fail fast if mismatched.

As per coding guidelines, "Cross-user data sharing is a bug; user data must never leak between signed-in accounts."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/run-codemode-registry.ts` around lines 335 - 347, The
direct use of `options.capabilityRegistry.capabilityMap` in `buildCodemodeFns`
trusts an externally supplied registry without confirming it belongs to the
current `callerContext`. Add a lightweight validation before using the prebuilt
registry, ideally by checking a user identity marker on
`BuiltCapabilityRegistry` against `callerContext.user?.userId`, and fail fast on
mismatch. Keep the fallback path through `getCapabilityRegistryForContext`
unchanged so only the injected `capabilityRegistry` path is guarded.

Source: Coding guidelines

packages/worker/src/repo/published-source.ts (1)

92-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated source-resolution/ownership-validation block.

The pre-fetched-source resolution + ownership/id-match checks (lines 94-102 and 137-145) are identical in both functions. Since this logic is security-sensitive (ownership check), consider extracting a shared helper to keep both call sites in sync if the validation rules change.

♻️ Proposed refactor
+async function resolveAndValidateSource(input: {
+	env: Env
+	userId: string
+	sourceId: string
+	source?: EntitySourceRow
+}): Promise<EntitySourceRow> {
+	const source =
+		input.source ??
+		(await getEntitySourceById(input.env.APP_DB, input.sourceId))
+	if (!source || source.user_id !== input.userId) {
+		throw new Error(`Published source "${input.sourceId}" was not found.`)
+	}
+	if (input.source && input.source.id !== input.sourceId) {
+		throw new Error(`Published source "${input.sourceId}" was not found.`)
+	}
+	return source
+}

Then in both loadPublishedEntitySource and loadPublishedEntityManifest, replace the inline block with const source = await resolveAndValidateSource(input).

Also applies to: 135-145

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/repo/published-source.ts` around lines 92 - 102, The
source resolution and ownership validation logic is duplicated in both
loadPublishedEntitySource and loadPublishedEntityManifest, which risks the two
call sites drifting out of sync. Extract the shared pre-fetched-source
resolution and validation into a helper such as resolveAndValidateSource, using
the existing input fields and checks around getEntitySourceById, source.user_id,
source.id, and sourceId, then replace both inline blocks with a single call to
that helper.
packages/worker/src/package-runtime/package-app.ts (1)

1406-1463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider exposing a cache-reset hook for packageAppWorkerOptionsCache.

The cache is module-level global state with no exported reset/clear helper, unlike the pattern used for other new caches in this PR stack (e.g. snapshot-cache.ts exposes invalidate/reset helpers). Current tests avoid collisions only by using unique userId/identity fields per test; a shared reset helper would make test isolation more robust against future test additions that might reuse identity fields.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/package-runtime/package-app.ts` around lines 1406 - 1463,
The module-level packageAppWorkerOptionsCache in buildPackageAppWorker is
missing a public way to clear cached entries, which makes test isolation
fragile. Add an exported reset/clear helper alongside
packageAppWorkerOptionsCache usage in package-app.ts, following the cache helper
pattern used by snapshot-cache.ts, and make tests call it in their
setup/teardown so they don’t rely only on unique userId or identity fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/worker/src/mcp/secrets/service.ts`:
- Around line 188-221: Promise.all in the secret lookup flow is causing
fail-fast behavior across scopes, so a bad lower-precedence entry can reject the
whole request even after a higher-priority match succeeds. Update the lookup
logic in the scope resolution path to preserve short-circuit precedence, either
by restoring sequential iteration over scopes or by using Promise.allSettled and
returning the first fulfilled result. Keep the fix localized around the
scopeResults handling in the secret service lookup and ensure session/app/user
precedence still returns the earliest valid secret.

In `@packages/worker/src/package-runtime/package-app.node.test.ts`:
- Around line 388-451: The fast path in resolvePersistablePackageSource() is
allowing a source to be reused without validating source.user_id against the
current userId. Update this path so it rejects mismatched ownership instead of
returning the provided source, and make buildPackageAppWorker reflect that
behavior. Add or adjust a test around
buildPackageAppWorker/resolvePersistablePackageSource that passes a source with
a different user_id and asserts the flow fails rather than persisting published
bundle artifacts.

In `@packages/worker/src/package-runtime/package-app.ts`:
- Around line 1178-1192: The fast path in resolvePersistablePackageSource
currently bypasses the ownership validation that the getEntitySourceById branch
enforces, allowing a source owned by another user to be reused. Update
resolvePersistablePackageSource so the early return only happens after verifying
input.source.user_id matches input.userId (not just that user_id and repo_id are
present), keeping the same ownership check as the slow path before any source is
returned to persistPublishedBundleArtifact.

---

Outside diff comments:
In `@packages/worker/src/package-registry/source.ts`:
- Around line 143-179: `loadPackageSourceBySourceId` re-reads the source row,
which can mix snapshots between manifest and file loading. Update this function
to accept an optional resolved `source` and reuse it when provided instead of
always calling `resolvePackageSourceRow`. Then thread `packageManifest.source`
from `loadSourceFiles` in `package-app.ts` into `loadPackageSourceBySourceId` so
the manifest and file reads stay on the same snapshot.

---

Nitpick comments:
In `@packages/worker/src/mcp/run-codemode-registry.ts`:
- Around line 335-347: The direct use of
`options.capabilityRegistry.capabilityMap` in `buildCodemodeFns` trusts an
externally supplied registry without confirming it belongs to the current
`callerContext`. Add a lightweight validation before using the prebuilt
registry, ideally by checking a user identity marker on
`BuiltCapabilityRegistry` against `callerContext.user?.userId`, and fail fast on
mismatch. Keep the fallback path through `getCapabilityRegistryForContext`
unchanged so only the injected `capabilityRegistry` path is guarded.

In `@packages/worker/src/mcp/secrets/service.node.test.ts`:
- Line 2: Add a regression test around resolveSecret in service.node.test.ts
that seeds a higher-precedence scope (session) with a valid secret and a
lower-precedence scope (user or app) with a corrupted entry that fails
decryption; then assert the call still returns the session value instead of
rejecting. Use the existing listSecrets, resolveSecret, and saveSecret test
helpers to locate the flow, and make sure the malformed encrypted_value only
affects the lower-precedence scope so the test covers scope-precedence behavior
under Promise.all.

In `@packages/worker/src/package-runtime/package-app.ts`:
- Around line 1406-1463: The module-level packageAppWorkerOptionsCache in
buildPackageAppWorker is missing a public way to clear cached entries, which
makes test isolation fragile. Add an exported reset/clear helper alongside
packageAppWorkerOptionsCache usage in package-app.ts, following the cache helper
pattern used by snapshot-cache.ts, and make tests call it in their
setup/teardown so they don’t rely only on unique userId or identity fields.

In `@packages/worker/src/repo/published-source.ts`:
- Around line 92-102: The source resolution and ownership validation logic is
duplicated in both loadPublishedEntitySource and loadPublishedEntityManifest,
which risks the two call sites drifting out of sync. Extract the shared
pre-fetched-source resolution and validation into a helper such as
resolveAndValidateSource, using the existing input fields and checks around
getEntitySourceById, source.user_id, source.id, and sourceId, then replace both
inline blocks with a single call to that helper.
🪄 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 Plus

Run ID: b4128f68-3753-41f9-826e-3e6fc1e4a683

📥 Commits

Reviewing files that changed from the base of the PR and between ea13953 and 6d8a223.

📒 Files selected for processing (31)
  • packages/worker/src/app/handlers/package-app.node.test.ts
  • packages/worker/src/app/handlers/package-app.ts
  • packages/worker/src/mcp/capabilities/registry.node.test.ts
  • packages/worker/src/mcp/capabilities/registry.ts
  • packages/worker/src/mcp/capabilities/remote-connector/index.ts
  • packages/worker/src/mcp/executor.node.test.ts
  • packages/worker/src/mcp/executor.ts
  • packages/worker/src/mcp/fetch-gateway.ts
  • packages/worker/src/mcp/run-codemode-registry.node.test.ts
  • packages/worker/src/mcp/run-codemode-registry.ts
  • packages/worker/src/mcp/secrets/capability-inputs.node.test.ts
  • packages/worker/src/mcp/secrets/capability-inputs.ts
  • packages/worker/src/mcp/secrets/crypto.node.test.ts
  • packages/worker/src/mcp/secrets/crypto.ts
  • packages/worker/src/mcp/secrets/service.node.test.ts
  • packages/worker/src/mcp/secrets/service.ts
  • packages/worker/src/mcp/tools/execute.node.test.ts
  • packages/worker/src/mcp/tools/execute.ts
  • packages/worker/src/mcp/values/repo.ts
  • packages/worker/src/mcp/values/service.node.test.ts
  • packages/worker/src/mcp/values/service.ts
  • packages/worker/src/package-registry/source.ts
  • packages/worker/src/package-runtime/module-graph.node.test.ts
  • packages/worker/src/package-runtime/module-graph.ts
  • packages/worker/src/package-runtime/package-app.node.test.ts
  • packages/worker/src/package-runtime/package-app.ts
  • packages/worker/src/package-runtime/published-bundle-artifacts.node.test.ts
  • packages/worker/src/remote-connector/client.ts
  • packages/worker/src/remote-connector/snapshot-cache.node.test.ts
  • packages/worker/src/remote-connector/snapshot-cache.ts
  • packages/worker/src/repo/published-source.ts

Comment thread packages/worker/src/mcp/secrets/service.ts Outdated
Comment thread packages/worker/src/package-runtime/package-app.node.test.ts
Comment thread packages/worker/src/package-runtime/package-app.ts

@cursor cursor 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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

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 2b38b9a. Configure here.

Comment thread packages/worker/src/remote-connector/snapshot-cache.ts

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
packages/worker/src/mcp/secrets/service.node.test.ts (1)

284-327: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Good coverage for the lower-precedence-corruption case; consider adding the mirror test.

This test correctly validates that a corrupted user-scope entry doesn't mask a valid session-scope result, matching resolveSecret's intent. However, the sibling scenario — a corrupted entry at the winning (highest-precedence available) scope must still surface as an error rather than silently returning found: false — doesn't appear to be covered in the shown range. That's exactly the case called out in resolveSecret's comment: Preserve sequential precedence semantics: a lower-precedence failure must not mask a higher-precedence hit, and a failure at the winning scope still surfaces as an error.

Consider adding a test where only the highest-precedence (session) entry exists and is corrupted, asserting resolveSecret rejects.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/worker/src/mcp/secrets/service.node.test.ts` around lines 284 - 327,
Add the missing mirror test for resolveSecret’s precedence behavior: the current
test in service.node.test.ts covers a corrupted lower-precedence user entry
being ignored when session wins, but it does not verify that a corrupted winning
session entry still throws. Use the existing resolveSecret, saveSecret, and
corruptSessionSecret/corrupt entry helpers in the same test file to create only
the highest-precedence available scope, corrupt it, and assert resolveSecret
rejects instead of returning found: false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/worker/src/mcp/secrets/service.node.test.ts`:
- Around line 284-327: Add the missing mirror test for resolveSecret’s
precedence behavior: the current test in service.node.test.ts covers a corrupted
lower-precedence user entry being ignored when session wins, but it does not
verify that a corrupted winning session entry still throws. Use the existing
resolveSecret, saveSecret, and corruptSessionSecret/corrupt entry helpers in the
same test file to create only the highest-precedence available scope, corrupt
it, and assert resolveSecret rejects instead of returning found: false.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d62447e5-31cd-4765-8c5d-c41b580e9408

📥 Commits

Reviewing files that changed from the base of the PR and between 6d8a223 and 2b38b9a.

📒 Files selected for processing (4)
  • packages/worker/src/mcp/secrets/service.node.test.ts
  • packages/worker/src/mcp/secrets/service.ts
  • packages/worker/src/package-runtime/package-app.node.test.ts
  • packages/worker/src/package-runtime/package-app.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/worker/src/mcp/secrets/service.ts
  • packages/worker/src/package-runtime/package-app.node.test.ts
  • packages/worker/src/package-runtime/package-app.ts

cursoragent and others added 7 commits July 4, 2026 17:58
- Package apps: read publish-time bundle artifacts via canonical identity
  (was a mismatched KV key that never hit), manifest-first fast path that
  skips loading full package source on artifact hit, parallel host setup,
  and isolate-level worker stub memoization keyed by caller identity
- MCP execute: reuse dynamic worker isolates for bundled modules via
  stable content-hashed worker IDs, pass the already-loaded capability
  registry through instead of rebuilding it, single-pass result
  serialization
- Module graph: parallelize dynamic import hydration and direct kody
  dependency resolution, memoize the runtime virtual module source
- Remote connectors: 30s isolate-level snapshot cache shared by registry
  synthesis, MCP init, and clients; memoize the built capability registry
  per user + connector snapshot identity
- Secrets: cache derived CryptoKeys, parallelize secret placeholder and
  scope resolution
- Values: replace per-bucket N+1 metadata queries with one JOIN query
- Dedupe duplicate entity-source row fetches in package source loading

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
… caching

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Worker-loader stubs are request-bound, so caching the stub itself broke
the second request to a package app. Cache the built worker options
instead and re-acquire a stub per request through APP_LOADER.get with a
stable content-hashed worker id so warm isolates are still reused.

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
- resolveSecret now uses Promise.allSettled so a corrupted lower-
  precedence entry cannot fail a resolved higher-precedence secret,
  while an error at the winning scope still surfaces.
- resolvePersistablePackageSource fast path now requires the pre-
  resolved source row to belong to the requesting user, matching the
  DB-lookup branch. Regression tests added for both.

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Bugbot flagged that a cached connected snapshot could serve stale
tools for up to the 30s TTL after a disconnect. Failed listTools or
callTool RPCs now invalidate the cached snapshot so the next registry
build re-fetches connector state.

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
DO stub RPC return types do not expose .catch through the RPC proxy
typing, which failed typecheck.

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
…st-scoped detail loads, throttled session refresh

Post-merge follow-up to the Remix 3 SSR work (#601):

- handleRequest builds the app router and parses env once per env object
  identity instead of on every request (~40 handler factories + schema
  validation were re-run per request).
- loadAdminUsersData issues the COUNT and page queries in parallel.
- loadCommunityDetailData memoizes per Request so the HTML handler and
  the frame renderer share one load during a single SSR response.
- Navigation-triggered client session refreshes are throttled to 30s;
  hydration and explicit refreshes (login/logout/profile) still always
  hit /session.

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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.

2 participants