Repository navigation
Add package services runtime - #264
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces package services—a new capability for saved packages to declare and manage long-lived background runtimes via Changes
Sequence DiagramsequenceDiagram
participant Client
participant MCP as MCP Server
participant DO as PackageServiceInstance<br/>(Durable Object)
participant Manifest as Package Manifest
participant Executor as Bundled Executor
participant Storage as Durable Storage
Client->>MCP: service_start(service_name, package_id)
MCP->>MCP: requirePackageServiceContext()
MCP->>DO: POST /service/start
DO->>Storage: Load persisted state
alt State is running
DO->>MCP: {ok: false, already_running: true}
else State is idle
DO->>DO: Transition to running<br/>Record runId & timestamps
DO->>Storage: Persist state
DO->>Manifest: Load service entry
DO->>Executor: waitUntil(execute bundled<br/>service module)
DO->>MCP: {ok: true, run_id, status: running}
end
rect rgba(100, 150, 200, 0.5)
Note over Executor,Storage: Background Execution (parallel)
Executor->>Executor: Load saved package source
Executor->>Executor: Build published bundle
Executor->>Executor: Create MCP context
Executor->>Executor: Execute service module<br/>with service tools
Executor->>Storage: Persist lastResult/<br/>lastError & timestamps
Executor->>Storage: Clear alarms on stop
end
Client->>MCP: service_get(service_name, package_id)
MCP->>DO: POST /service/status
DO->>Storage: Load current status
DO->>MCP: {status, active_run_id, ...metadata}
MCP->>Client: {status, ...}
Client->>MCP: service_stop(service_name, package_id)
MCP->>DO: POST /service/stop
DO->>Storage: Load state
DO->>Storage: Set stopRequested=true<br/>Update status to stopping
DO->>DO: Clear alarms
DO->>MCP: {ok: true}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
kentcdodds
marked this pull request as ready for review
April 24, 2026 22:40
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/worker/src/package-runtime/published-runtime-artifacts.ts (1)
52-70:⚠️ Potential issue | 🟡 MinorLegacy artifacts will deserialize with
serviceContext: undefined, notnull.
bundleArtifactVersionremains at 1, andreadPublishedBundleArtifactspreads the stored artifact without normalizing missing fields. Any artifact written before this PR will return withserviceContextabsent, resulting inundefined, while the type declares{ serviceName: string } | null. This violates the type contract and can cause downstream checks or destructuring with defaultnullvalues to behave inconsistently.Normalize on read to ensure the field is always
nullwhen absent, matching the pattern used inmodule-graph.ts:Suggested fix
return { ...artifact, + serviceContext: artifact.serviceContext ?? null, modules: deserializeWorkerLoaderModules(artifact.modules), } satisfies PublishedBundleArtifact🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/published-runtime-artifacts.ts` around lines 52 - 70, Legacy artifacts may omit serviceContext and deserialize it as undefined, violating the PublishedBundleArtifact type; update readPublishedBundleArtifact to normalize missing or undefined serviceContext to null (matching the module-graph.ts pattern) so that returned objects always have serviceContext: { serviceName: string } | null; ensure other optional fields follow the same normalization and keep bundleArtifactVersion and PublishedBundleArtifact type unchanged.packages/worker/src/repo/checks.ts (1)
403-405:⚠️ Potential issue | 🟡 MinorBundle-check message is stale — services are now bundled too.
This message still says "no app entry, exports, or jobs to bundle." Services are now collected as additional bundle targets (lines 257-265), so the message should mention them for accuracy when a manifest with only services would otherwise look empty.
📝 Proposed wording
- return 'Package defines no app entry, exports, or jobs to bundle.' + return 'Package defines no app entry, exports, jobs, or services to bundle.'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/repo/checks.ts` around lines 403 - 405, The bundle-empty message is outdated: when input.targetCount === 0 the returned string only mentions "app entry, exports, or jobs" but services are now included as bundle targets (see the services collection around the service-target logic), so update the return value in checks.ts to include services (e.g., mention "services" alongside app entry, exports, and jobs) so the message accurately reflects all possible bundle targets; locate the check using input.targetCount and replace the stale message string accordingly.
🧹 Nitpick comments (7)
packages/worker/src/package-runtime/package-service.node.test.ts (1)
1-11: LGTM — consider extending coverage for collision-prone characters.The current cases cover the happy path and whitespace encoding. Optional: add a case for a
serviceNamecontaining:or/to lock in that the encoded id remains unambiguous against theservice:<packageId>:<serviceName>separator (sincepackageIdis concatenated unencoded and could otherwise collide).♻️ Optional extra assertion
expect(buildPackageServiceStorageId('package-1', 'guild sync')).toBe( 'service:package-1:guild%20sync', ) + expect(buildPackageServiceStorageId('package-1', 'a:b/c')).toBe( + 'service:package-1:a%3Ab%2Fc', + ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/package-service.node.test.ts` around lines 1 - 11, Add tests to assert that buildPackageServiceStorageId handles collision-prone characters by percent-encoding service names containing separators like ':' and '/', e.g., call buildPackageServiceStorageId with serviceName values that include ':' and '/' and assert the returned ID is 'service:<packageId>:<percent-encoded-serviceName>' (verify ':' becomes %3A and '/' becomes %2F) so the concatenated form remains unambiguous; update the test file to include these extra expect(...) assertions referencing buildPackageServiceStorageId.packages/worker/src/package-runtime/published-bundle-artifacts.ts (1)
43-54: Remove the unusedserviceContextfield from the persisted artifact shape.The
serviceContextfield hardcoded tonull(line 174) is never read by consumers. Bothpackage-service.ts(lines 219–221) and job execution constructserviceContextfresh at runtime from binding or manifest data, then pass it directly torunBundledModuleWithRegistry. The persisted field is dead weight and should be removed to avoid schema confusion.Either:
- Delete
serviceContext: nullfrom the persisted shape and the type field.- Or, if intended for future use, add it to
PersistPublishedBundleArtifactInputand thread it through the service-iteration loop (lines 299–318) so the field carries actual data.Option 1: Remove the field entirely
type PublishedBundleArtifact = { env: Env userId: string source: EntitySourceRow kind: BundleArtifactKind artifactName?: string | null entryPoint: string mainModule: string modules: WorkerLoaderModules dependencies: Array<BundleArtifactDependency> packageContext: PublishedBundleArtifactPackageContext | null - serviceContext: PublishedBundleArtifactServiceContext | null createdAt: string }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/published-bundle-artifacts.ts` around lines 43 - 54, Persisted artifact shape includes an unused serviceContext field; remove it by deleting serviceContext from the PersistPublishedBundleArtifactInput type and removing the hardcoded serviceContext: null assignment where the persisted object is created (reference symbol: PersistPublishedBundleArtifactInput and the persisted artifact construction that sets serviceContext: null), and also remove any code in the service-iteration loop that expects or threads this persisted field (references: the service-iteration loop that later reconstructs serviceContext and the consumer call runBundledModuleWithRegistry), so serviceContext is only created at runtime and not stored in the persisted shape.packages/worker/src/repo/checks.ts (1)
257-265: Redundant entry-path lookup.
listPackageServices(manifest)already returnsentrynormalized vianormalizePackageWorkspacePath(perpackages/worker/src/package-registry/manifest.ts:130-138), so callinggetPackageServiceEntryPath({ manifest, serviceName })here just re-derives the same value. You can useservice.entrydirectly and drop thegetPackageServiceEntryPathimport (and theifguard, sincelistPackageServicesonly emits services with a defined entry per the schema).♻️ Proposed simplification
for (const service of listPackageServices(manifest)) { - const serviceEntryPath = getPackageServiceEntryPath({ - manifest, - serviceName: service.name, - }) - if (serviceEntryPath) { - remember(serviceEntryPath, true) - } + remember(service.entry, true) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/repo/checks.ts` around lines 257 - 265, Replace the redundant lookup: use the entry already provided by listPackageServices by calling remember(service.entry, true) for each service (iterate over listPackageServices(manifest) and reference service.entry), remove the getPackageServiceEntryPath call and its conditional guard, and delete the now-unused getPackageServiceEntryPath import; keep manifest and remember as-is.packages/worker/src/mcp/capabilities/services/services-domain.node.test.ts (1)
76-102: Mock returns arpcfield that isn't part of the real API.
listSavedPackageServices(inpackages/worker/src/package-runtime/package-service.ts) returns only{ savedPackage, services }. Therpc(...)builder is added bylistPackageServicesForContextinshared.ts. Including it on the mockedlistSavedPackageServicesvalue misrepresents the contract and is dead code in the test (the realpackageServiceRpcmock is what gets exercised).Consider trimming the mock to match the real return shape so the test stays aligned with the source:
mockModule.listSavedPackageServices.mockResolvedValue({ savedPackage: { id: 'package-123', kodyId: 'example', }, services: [ { name: 'discord-gateway', entry: 'services/discord-gateway.ts', autoStart: true, }, ], - rpc: () => ({ - status: async () => ({ ... }), - }), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/services/services-domain.node.test.ts` around lines 76 - 102, The mock for listSavedPackageServices is returning an extra rpc builder that doesn't exist on the real API; remove the rpc field from the mocked resolved value so it matches the actual return shape ({ savedPackage, services }) and let listPackageServicesForContext / packageServiceRpc provide any RPC behavior in other mocks; update the mockModule.listSavedPackageServices.mockResolvedValue call to only include savedPackage and services (e.g., id/kodyId and the services array with name/entry/autoStart) to keep the test aligned with the real package-service.ts contract.packages/worker/src/package-runtime/package-service.ts (1)
197-231: Minor: extractstorageIdto avoid recomputation.
buildPackageServiceStorageId(binding.packageId, binding.serviceName)is computed twice inrunService(lines 200-204 and 222-227). Easy to drift if one site is updated. Pull it into a local once:+const storageId = buildPackageServiceStorageId( + binding.packageId, + binding.serviceName, +) const callerContext = createMcpCallerContext({ baseUrl: binding.baseUrl, user: { ... }, storageContext: { sessionId: null, appId: binding.packageId, - storageId: buildPackageServiceStorageId( - binding.packageId, - binding.serviceName, - ), + storageId, }, }) ... storageTools: { userId: binding.userId, - storageId: buildPackageServiceStorageId( - binding.packageId, - binding.serviceName, - ), + storageId, writable: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/package-service.ts` around lines 197 - 231, The code calls buildPackageServiceStorageId(binding.packageId, binding.serviceName) twice in runService; extract it to a local const (e.g., const storageId = buildPackageServiceStorageId(binding.packageId, binding.serviceName)) and replace both uses: the storageContext.storageId and the storageTools.storageId passed into runBundledModuleWithRegistry; ensure the same storageId variable is used in both places so the value isn’t recomputed or drift between sites.packages/worker/src/mcp/capabilities/services/service-get.ts (1)
19-35: Misleading "not found" error and minor consistency note.The
servicefield onrequirePackageServiceContext's return value isnullonly whenserviceName.trim()is empty. Sincez.string().min(1)accepts whitespace-only strings (it checks length, not trimmed length), a value like' 'will pass schema validation, reach line 31, and surface asPackage service " " was not found., even though the real cause is an empty name and not a missing manifest entry.Two small improvements worth considering:
- Tighten the input schema (or reuse
requirePackageServiceContext's trim) to reject whitespace-only names up front.- For symmetry with
service-listwhich runsnormalizePackageServiceStatus(...), consider validating the raw.status()payload through the same helper before returning so output validation isn't solely dependent on the framework usingoutputSchema.Possible diff
+import { normalizePackageServiceStatus, packageServiceStatusSchema, requirePackageServiceContext } from './shared.ts' @@ inputSchema: z.object({ - service_name: z.string().min(1), + service_name: z.string().trim().min(1), package_id: z.string().min(1).optional(), }), @@ - return await serviceContext.service.status() + return normalizePackageServiceStatus(await serviceContext.service.status())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/services/service-get.ts` around lines 19 - 35, Tighten validation and normalize output: change the inputSchema for this handler to trim and reject whitespace-only names (e.g., transform/trim then refine non-empty) so requirePackageServiceContext never receives blank serviceName, and in the handler use the same normalization used by service-list (normalizePackageServiceStatus) to validate/normalize the raw service.status() payload before returning (ensuring output conforms to packageServiceStatusSchema); reference inputSchema, handler, requirePackageServiceContext, normalizePackageServiceStatus, and packageServiceStatusSchema when making these changes.packages/worker/src/mcp/capabilities/services/shared.ts (1)
142-146:buildPackageServiceBindingis a no-op wrapper.This function just returns
input.bindingunchanged — no validation, no transformation, no branded type. As written it's pure indirection and doesn't justify its export surface. Either delete it (callers can pass the binding object directly) or have it actually validate / freeze / brand the binding to add value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/services/shared.ts` around lines 142 - 146, buildPackageServiceBinding is currently a no-op that just returns input.binding (PackageServiceBindingState); remove the useless wrapper or make it meaningful. Option A: delete the exported function and update all callers to use the PackageServiceBindingState object directly. Option B: implement actual validation/branding in buildPackageServiceBinding: validate required properties on input.binding, coerce/normalize values as needed, freeze the object (Object.freeze) and return it as a branded type (e.g., cast/return as PackageServiceBinding) so callers get runtime guarantees; then keep the export and adjust types where necessary. Ensure you update every caller of buildPackageServiceBinding (or their imports) accordingly.
🤖 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/capabilities/services/service-list.ts`:
- Around line 38-48: The current service listing uses Promise.all over
listed.services.map and will reject if any listed.rpc(service.name).status()
throws; change this to tolerate per-service failures by using Promise.allSettled
(or wrapping each status call in try/catch) and, for failed entries, set the
returned object's status field (the one currently assigned from
normalizePackageServiceStatus(...).status) to a fallback like 'unknown' or the
error message; update the mapping that builds { name, entry, auto_start, status
} so healthy services still appear even if one status call fails.
In `@packages/worker/src/mcp/capabilities/services/service-start.ts`:
- Around line 38-43: service_stop currently uses optional chaining
(serviceContext.service?.stop()) and hides "service not found" cases, while
service_start throws when serviceContext.service is null; make them symmetric by
changing service_stop to explicitly check serviceContext.service and throw the
same Error used in service_start when null (use the same message that references
args.service_name), otherwise call and await serviceContext.service.stop() and
return its result—update service_stop function to mirror service_start's
missing-service handling (referencing service_stop, service_start, and
serviceContext.service).
In `@packages/worker/src/package-runtime/package-app.ts`:
- Around line 704-716: The runtime serviceList() currently returns services from
listSavedPackageServices without the live status field, causing a mismatch with
MCP's service_list; update serviceList (in package-app.ts) to iterate over
result.services and for each service call rpc(service.name).status() (or await
this.rpc(service.name).status()) to populate a status property before returning,
ensuring the returned object includes package_id, kody_id and services with
name, entry, autoStart and status; alternatively, if you choose not to enrich,
add clear documentation in runtime.services.list() noting the intentional
absence of live status.
In `@packages/worker/src/package-runtime/package-service.ts`:
- Around line 293-315: handleStatusRequest is reloading the package manifest on
every call via loadSavedPackageService to read autoStart and also contains a
dead null fallback because initializeBinding guarantees
this.stateSnapshot.binding is set; fix by caching/resolving the
serviceDefinition.autoStart into this.stateSnapshot (or another cached field)
during initializeBinding or on first start so handleStatusRequest reads
autoStart from this.stateSnapshot instead of calling loadSavedPackageService,
remove the redundant ternary null branch that checks this.stateSnapshot.binding
after initializeBinding, and ensure handleStatusRequest returns status even if
manifest lookup fails so it degrades gracefully when the package source is
temporarily unavailable.
- Around line 248-291: handleStartRequest currently awaits runService with no
cancellation and overwrites a user-initiated stop; add cooperative cancellation
and a stop flag: modify handleStopRequest to set stateSnapshot.stopRequested =
true (persist) and call alarm delete as today, then thread an AbortSignal into
runService and into runBundledModuleWithRegistry so runService accepts an
AbortSignal param (or an options object) and cancels work when signaled (use
AbortController in the stop handler to signal the running start). At the end of
handleStartRequest, after runService resolves or rejects, check
stateSnapshot.stopRequested and, if true, ensure the final persisted status
reflects that an explicit stop was requested (do not overwrite it with a
natural-success result), clear stopRequested as appropriate, and persist state;
ensure names to change/inspect include handleStartRequest, handleStopRequest,
runService, runBundledModuleWithRegistry, and stateSnapshot.stopRequested.
- Around line 85-90: The storage id assembly in buildPackageServiceStorageId is
asymmetric: packageId is used raw while serviceName is
encodeURIComponent-encoded, which allows ambiguous ids when packageId contains
delimiters; update buildPackageServiceStorageId to encode both packageId and
serviceName (e.g., call encodeURIComponent on packageId as well) so the returned
string `service:${encodedPackageId}:${encodedServiceName}` is unambiguous.
In `@packages/worker/wrangler.jsonc`:
- Around line 119-122: The preview and test environment blocks are missing the
Durable Object binding for PackageServiceInstance (PACKAGE_SERVICE_INSTANCE),
causing runtime failures; add an entry identical to the production binding
(class_name "PackageServiceInstance", name "PACKAGE_SERVICE_INSTANCE") into both
the preview and test environment "bindings" arrays so the Durable Object is
available in all environments and matches the v14 migration and tests that
reference PACKAGE_SERVICE_INSTANCE.
---
Outside diff comments:
In `@packages/worker/src/package-runtime/published-runtime-artifacts.ts`:
- Around line 52-70: Legacy artifacts may omit serviceContext and deserialize it
as undefined, violating the PublishedBundleArtifact type; update
readPublishedBundleArtifact to normalize missing or undefined serviceContext to
null (matching the module-graph.ts pattern) so that returned objects always have
serviceContext: { serviceName: string } | null; ensure other optional fields
follow the same normalization and keep bundleArtifactVersion and
PublishedBundleArtifact type unchanged.
In `@packages/worker/src/repo/checks.ts`:
- Around line 403-405: The bundle-empty message is outdated: when
input.targetCount === 0 the returned string only mentions "app entry, exports,
or jobs" but services are now included as bundle targets (see the services
collection around the service-target logic), so update the return value in
checks.ts to include services (e.g., mention "services" alongside app entry,
exports, and jobs) so the message accurately reflects all possible bundle
targets; locate the check using input.targetCount and replace the stale message
string accordingly.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/services/service-get.ts`:
- Around line 19-35: Tighten validation and normalize output: change the
inputSchema for this handler to trim and reject whitespace-only names (e.g.,
transform/trim then refine non-empty) so requirePackageServiceContext never
receives blank serviceName, and in the handler use the same normalization used
by service-list (normalizePackageServiceStatus) to validate/normalize the raw
service.status() payload before returning (ensuring output conforms to
packageServiceStatusSchema); reference inputSchema, handler,
requirePackageServiceContext, normalizePackageServiceStatus, and
packageServiceStatusSchema when making these changes.
In `@packages/worker/src/mcp/capabilities/services/services-domain.node.test.ts`:
- Around line 76-102: The mock for listSavedPackageServices is returning an
extra rpc builder that doesn't exist on the real API; remove the rpc field from
the mocked resolved value so it matches the actual return shape ({ savedPackage,
services }) and let listPackageServicesForContext / packageServiceRpc provide
any RPC behavior in other mocks; update the
mockModule.listSavedPackageServices.mockResolvedValue call to only include
savedPackage and services (e.g., id/kodyId and the services array with
name/entry/autoStart) to keep the test aligned with the real package-service.ts
contract.
In `@packages/worker/src/mcp/capabilities/services/shared.ts`:
- Around line 142-146: buildPackageServiceBinding is currently a no-op that just
returns input.binding (PackageServiceBindingState); remove the useless wrapper
or make it meaningful. Option A: delete the exported function and update all
callers to use the PackageServiceBindingState object directly. Option B:
implement actual validation/branding in buildPackageServiceBinding: validate
required properties on input.binding, coerce/normalize values as needed, freeze
the object (Object.freeze) and return it as a branded type (e.g., cast/return as
PackageServiceBinding) so callers get runtime guarantees; then keep the export
and adjust types where necessary. Ensure you update every caller of
buildPackageServiceBinding (or their imports) accordingly.
In `@packages/worker/src/package-runtime/package-service.node.test.ts`:
- Around line 1-11: Add tests to assert that buildPackageServiceStorageId
handles collision-prone characters by percent-encoding service names containing
separators like ':' and '/', e.g., call buildPackageServiceStorageId with
serviceName values that include ':' and '/' and assert the returned ID is
'service:<packageId>:<percent-encoded-serviceName>' (verify ':' becomes %3A and
'/' becomes %2F) so the concatenated form remains unambiguous; update the test
file to include these extra expect(...) assertions referencing
buildPackageServiceStorageId.
In `@packages/worker/src/package-runtime/package-service.ts`:
- Around line 197-231: The code calls
buildPackageServiceStorageId(binding.packageId, binding.serviceName) twice in
runService; extract it to a local const (e.g., const storageId =
buildPackageServiceStorageId(binding.packageId, binding.serviceName)) and
replace both uses: the storageContext.storageId and the storageTools.storageId
passed into runBundledModuleWithRegistry; ensure the same storageId variable is
used in both places so the value isn’t recomputed or drift between sites.
In `@packages/worker/src/package-runtime/published-bundle-artifacts.ts`:
- Around line 43-54: Persisted artifact shape includes an unused serviceContext
field; remove it by deleting serviceContext from the
PersistPublishedBundleArtifactInput type and removing the hardcoded
serviceContext: null assignment where the persisted object is created (reference
symbol: PersistPublishedBundleArtifactInput and the persisted artifact
construction that sets serviceContext: null), and also remove any code in the
service-iteration loop that expects or threads this persisted field (references:
the service-iteration loop that later reconstructs serviceContext and the
consumer call runBundledModuleWithRegistry), so serviceContext is only created
at runtime and not stored in the persisted shape.
In `@packages/worker/src/repo/checks.ts`:
- Around line 257-265: Replace the redundant lookup: use the entry already
provided by listPackageServices by calling remember(service.entry, true) for
each service (iterate over listPackageServices(manifest) and reference
service.entry), remove the getPackageServiceEntryPath call and its conditional
guard, and delete the now-unused getPackageServiceEntryPath import; keep
manifest and remember as-is.
🪄 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: 7070ed73-afb6-4e07-a485-5dc8e756e7f7
📒 Files selected for processing (30)
docs/contributing/architecture/request-lifecycle.mddocs/use/execute.mddocs/use/packages.mdpackages/worker/src/env-schema.tspackages/worker/src/index.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/services/domain.tspackages/worker/src/mcp/capabilities/services/service-get.tspackages/worker/src/mcp/capabilities/services/service-list.tspackages/worker/src/mcp/capabilities/services/service-start.tspackages/worker/src/mcp/capabilities/services/service-stop.tspackages/worker/src/mcp/capabilities/services/services-domain.node.test.tspackages/worker/src/mcp/capabilities/services/shared.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/mcp/server-instructions.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-registry/manifest.node.test.tspackages/worker/src/package-registry/manifest.tspackages/worker/src/package-registry/types.tspackages/worker/src/package-runtime/module-graph.tspackages/worker/src/package-runtime/package-app.tspackages/worker/src/package-runtime/package-service.node.test.tspackages/worker/src/package-runtime/package-service.tspackages/worker/src/package-runtime/published-bundle-artifacts.tspackages/worker/src/package-runtime/published-runtime-artifacts.tspackages/worker/src/repo/checks.tspackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonc
Contributor
|
🔎 Preview deployed: https://kody-pr-264.kentcdodds.workers.dev Worker: Mocks:
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/worker/src/package-runtime/package-service.ts (2)
256-265:⚠️ Potential issue | 🟠 MajorDead ternary — both branches set
statusto'stopped'.this.stateSnapshot.status = this.stateSnapshot.stopRequested ? 'stopped' : 'stopped'A natural completion of
runServiceis currently indistinguishable from a user-initiated stop, and (per the prior review on stop semantics) a late-finishing run will overwrite a realstoppedstate with anotherstoppedplus a freshlastResult/lastRunFinishedAt, hiding that an explicit stop ever happened.Pick distinct semantics for the two cases — e.g.,
'stopped'only whenstopRequested, otherwise something like'idle'(or a new'completed') — and skip overwritinglastResult/lastRunFinishedAtwhen a stop has already finalized state for this run.🔧 Suggested direction
- this.stateSnapshot.status = this.stateSnapshot.stopRequested - ? 'stopped' - : 'stopped' - this.stateSnapshot.currentRunId = null - this.stateSnapshot.stopRequested = false - this.stateSnapshot.lastResult = result - this.stateSnapshot.lastRunFinishedAt = new Date().toISOString() - this.stateSnapshot.lastStoppedAt = this.stateSnapshot.lastRunFinishedAt - await this.persistState() + const wasStopRequested = this.stateSnapshot.stopRequested + this.stateSnapshot.status = wasStopRequested ? 'stopped' : 'idle' + this.stateSnapshot.currentRunId = null + this.stateSnapshot.stopRequested = false + this.stateSnapshot.lastResult = result + this.stateSnapshot.lastRunFinishedAt = new Date().toISOString() + if (wasStopRequested) { + this.stateSnapshot.lastStoppedAt = this.stateSnapshot.lastRunFinishedAt + } + await this.persistState()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/package-service.ts` around lines 256 - 265, The ternary setting this.stateSnapshot.status currently sets 'stopped' in both branches; change it so when this.stateSnapshot.stopRequested is true the status remains 'stopped' but when false set a distinct state such as 'idle' or 'completed' (choose one and apply consistently). Also, in the completion path inside the same block (the code that sets this.stateSnapshot.lastResult and lastRunFinishedAt and clears currentRunId/stopRequested), do not overwrite lastResult/lastRunFinishedAt if stopRequested was true (i.e., only set lastResult/lastRunFinishedAt when stopRequested is false), then call await this.persistState(); keep references to this.stateSnapshot and the completion/stop logic in the same method so the semantics are unambiguous.
382-415:⚠️ Potential issue | 🟡 MinorStop still cannot interrupt an in-flight run.
handleStopRequestflipsstopRequestedand clears the alarm, butrunService/runBundledModuleWithRegistryis not given anAbortSignal, so a long-lived service module continues executing until it cooperatively pollsservice.shouldStop(). For genuinely long-lived runtimes (the stated PR goal) this is acceptable only ifshouldStopis documented as the cancellation contract; otherwise consider plumbing anAbortControllerthroughrunBundledModuleWithRegistry.At minimum, the
service.shouldStopcontract should be called out in the package authoring docs added in this PR so authors know they're responsible for honoring it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/package-service.ts` around lines 382 - 415, The stop flow doesn't actually abort the running module because runServiceInBackground/runBundledModuleWithRegistry aren't given an AbortSignal and rely only on service.shouldStop; fix by creating an AbortController when starting a run (in handleStartRequest), store its signal on the instance (e.g. this.activeRunController and this.activeRunPromise), and pass that AbortSignal into runServiceInBackground and onward into runBundledModuleWithRegistry so the bundled runtime can be force-aborted; then update handleStopRequest to call this.activeRunController?.abort() in addition to flipping stopRequested/clearing alarms. Alternatively, if you prefer cooperative cancellation only, add explicit documentation for the service.shouldStop contract in package authoring docs, but the code-level fix is to thread an AbortSignal through the symbols handleStartRequest, handleStopRequest, runServiceInBackground, and runBundledModuleWithRegistry.
🧹 Nitpick comments (7)
packages/worker/src/mcp/run-codemode-registry.node.test.ts (1)
926-939: Optional: prefer asserting on spy call args rather thanexpectinsidemockImplementation.Assertions inside
mockImplementationfire as a side effect of the call; if the production code ever wrapscreateExecuteExecutorinvocation in a try/catch, a failedexpectcould be swallowed. A more robust pattern is to let the mock return normally and then assert oncreateExecuteExecutorSpy.mock.calls[0][0].timeoutMsafter the awaited call. Non-blocking.♻️ Suggested pattern
- const createExecuteExecutorSpy = vi - .spyOn(await import('#mcp/executor.ts'), 'createExecuteExecutor') - .mockImplementation((input) => { - expect(input.timeoutMs).toBe(300_000) - return { - async execute(source) { - wrapped = String(source) - return { - result: 'ok', - logs: [], - } - }, - } as never - }) + const createExecuteExecutorSpy = vi + .spyOn(await import('#mcp/executor.ts'), 'createExecuteExecutor') + .mockImplementation(() => ({ + async execute(source) { + wrapped = String(source) + return { result: 'ok', logs: [] } + }, + }) as never)Then after
await runBundledModuleWithRegistry(...):expect(createExecuteExecutorSpy).toHaveBeenCalledWith( expect.objectContaining({ timeoutMs: 300_000 }), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/run-codemode-registry.node.test.ts` around lines 926 - 939, Remove the inline assertion from the mockImplementation of createExecuteExecutor (created as createExecuteExecutorSpy) and have the mock simply return the fake executor; after the test's awaited call to runBundledModuleWithRegistry, assert on the spy's call arguments instead (e.g., expect(createExecuteExecutorSpy).toHaveBeenCalledWith(expect.objectContaining({ timeoutMs: 300_000 }))). This keeps the mock free of side-effect assertions and uses createExecuteExecutorSpy.mock.calls (or Jest/Vi matcher) to validate the timeoutMs passed into createExecuteExecutor.docs/use/execute.md (2)
108-112: Consider reorganizing lifecycle details for clarity.The explanation of
service_startreturning immediately and background execution semantics (lines 110-112) is technically accurate but feels organizationally disconnected from storage concepts. While the information is correct, readers might benefit from a clearer separation between storage scoping (line 108-109) and lifecycle/execution model (lines 110-112).💡 Optional organizational improvement
Consider splitting this into two separate bullets or adding a subheading to distinguish storage semantics from execution lifecycle:
- bound storage is execute-, app-, package-, or job-owned durable state -- package service runs also get writable service-owned durable state scoped to - the declared service name -- package service runs are background-managed by the service Durable Object, so - `service_start` returns immediately with a running state while the service code - continues in the background until it finishes, errors, or cooperatively stops +- package service runs get writable service-owned durable state scoped to the + declared service name +- package service execution is background-managed: `service_start` returns + immediately with a running state while the service code continues in the + background until it finishes, errors, or cooperatively stops🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/use/execute.md` around lines 108 - 112, Split the combined bullet into two distinct parts: one bullet that explains storage scoping for "package service runs" (service-owned durable state scoped to the declared service name) and a separate bullet or subheading that describes the execution lifecycle semantics for service_start (that service_start returns immediately and the service runs in the background until finish/error/cooperative stop). Update the wording under the "package service runs" statement to only cover storage concerns and create a new bullet or a small subheading titled "Execution lifecycle" (or similar) to host the service_start background/execution details so storage and lifecycle are clearly separated.
27-32: Specify the parameter type forsetAlarm.The
setAlarmmethod accepts aDateparameter, but this isn't documented. Based on the implementation, users might not know whether to pass a Date object, timestamp, or ISO string, which could lead to confusion or errors.📝 Suggested enhancement
- - `await service.setAlarm(runAt)` — schedule the next service wake-up + - `await service.setAlarm(runAt)` — schedule the next service wake-up (pass a Date)Alternatively, for consistency with the brief style of this documentation:
- - `await service.setAlarm(runAt)` — schedule the next service wake-up + - `await service.setAlarm(runAt: Date)` — schedule the next service wake-upAs per relevant code snippet from
packages/worker/src/mcp/run-codemode-registry.ts:51-72, theServiceToolOptionstype signature showssetAlarm: (runAt: Date) => Promise<{ ok: true; scheduled_at: string }>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/use/execute.md` around lines 27 - 32, Update the docs to specify that service.setAlarm accepts a Date object: document the parameter as setAlarm(runAt: Date) and note the Promise return shape per ServiceToolOptions (Promise<{ ok: true; scheduled_at: string }>) so users know to pass a JavaScript Date and what the method returns; reference the implementation signature in ServiceToolOptions (see run-codemode-registry.ts) when editing the list item for setAlarm.packages/worker/src/package-runtime/package-service.ts (2)
463-480:alarm()is brittle and re-uses an HTTP-shaped helper internally.Two concerns:
loadSavedPackageServiceis called unconditionally; if the package source is briefly unavailable (KV/D1 hiccup), the alarm throws and Cloudflare may retry, but the auto-start intent is lost mid-window. Consider falling back to the cachedthis.stateSnapshot.autoStarton load failure.await this.handleStartRequest({ binding })constructs and discards aResponse.json(...). Extract the start logic into a private method that returns thePackageServiceRunResult(orvoid) and have bothhandleStartRequestandalarm()call it directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/package-service.ts` around lines 463 - 480, alarm() currently always calls loadSavedPackageService and then calls handleStartRequest which builds and discards an HTTP Response; change it so alarm() catches failures from loadSavedPackageService and falls back to this.stateSnapshot.autoStart (so auto-start intent isn't lost on transient KV/D1 errors), and refactor the start flow by extracting the core start logic from handleStartRequest into a new private method (e.g., private executeStart(binding): Promise<PackageServiceRunResult | void>) that returns the PackageServiceRunResult or void; have handleStartRequest call executeStart and build its Response.json around that result, and have alarm() call executeStart directly (no Response construction), while preserving the existing persistState() call and stopRequested checks.
439-461:fetchconsumes the request body before route matching.
await request.json()is called for every request, including unsupported methods/paths that fall through to404. This is harmless today but means any future GET endpoint (e.g., a health probe) added to the DO will fail since GET requests have no body. Consider matching the route first, then parsing the body only for handlers that need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/package-service.ts` around lines 439 - 461, The fetch method currently always calls await request.json(), consuming the body before route matching; change fetch to first inspect request.method and url.pathname and only parse the JSON body for the POST routes that need it (e.g., before calling handleStartRequest, handleStatusRequest, handleStopRequest). Keep the early binding check logic but move the body parsing into the branches that call handleStartRequest/handleStatusRequest/handleStopRequest; for the /service-status branch call buildServiceStatusResponse(binding) without parsing body, and return 404 for unmatched routes without touching request.json().packages/worker/src/mcp/capabilities/services/shared.ts (1)
91-126: Redundant package + source load on theservice_listpath.
listPackageServicesForContextfirst callsrequirePackageServiceContext(which executesgetSavedPackageByIdand, indirectly, the saved-package lookup), and then callslistSavedPackageServiceswhich does anothergetSavedPackageByIdplusloadPackageSourceBySourceIdfor the same(userId, packageId). For an MCPservice_listcall this means duplicate D1/source-bundle loads on every request. Consider either:
- Inlining the resolve+load steps here and skipping
listSavedPackageServices, or- Having
listSavedPackageServicesaccept a pre-resolvedsavedPackageso it only loads the source.packages/worker/src/repo/checks.ts (1)
201-210: Ambientservicetypes diverge from runtime contract.The prelude declares
setAlarm(runAt: string | Date)andgetStatus()/setAlarm()/clearAlarm()returningPromise<unknown>, but the actual runtime injected fromPackageServiceInstanceBase.runServiceInBackground(seepackages/worker/src/package-runtime/package-service.tslines 241-249) only acceptsDateforsetAlarmand returns concrete shapes ({ ok: true; scheduled_at: string }, etc.). Consider tightening these signatures (and exposinggetStatus's return shape — e.g., thepackageServiceStatusSchemafromshared.ts) so package authors get accurate IntelliSense and catch misuse at typecheck time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/repo/checks.ts` around lines 201 - 210, The ambient declaration for packageContext/serviceContext/service is too loose: update the `service` type used in checks.ts to match the runtime contract provided by `PackageServiceInstanceBase.runServiceInBackground` — make `setAlarm` accept only `Date` (not string | Date) and tighten the return types of `getStatus`, `setAlarm` and `clearAlarm` to the concrete shapes used at runtime (e.g., use the `packageServiceStatusSchema` shape for `getStatus` and the actual `{ ok: boolean; scheduled_at: string }`-like shape for `setAlarm`/`clearAlarm`), so package authors get accurate IntelliSense and typechecking.
🤖 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/package-runtime/package-service.ts`:
- Around line 188-197: scheduleAlarm currently assumes input.runAt is a Date and
calls input.runAt.toISOString(), which will throw if callers pass a string;
update scheduleAlarm (the method) to normalize runAt by checking typeof
input.runAt === 'string' and parsing it to a Date (or use new Date(input.runAt))
before calling toISOString(), then pass that Date to this.ctx.storage.setAlarm,
set this.stateSnapshot.nextAlarmAt to the ISO string, and await
this.persistState(); this keeps the declared service.setAlarm signature
(string|Date) working without runtime errors.
---
Duplicate comments:
In `@packages/worker/src/package-runtime/package-service.ts`:
- Around line 256-265: The ternary setting this.stateSnapshot.status currently
sets 'stopped' in both branches; change it so when
this.stateSnapshot.stopRequested is true the status remains 'stopped' but when
false set a distinct state such as 'idle' or 'completed' (choose one and apply
consistently). Also, in the completion path inside the same block (the code that
sets this.stateSnapshot.lastResult and lastRunFinishedAt and clears
currentRunId/stopRequested), do not overwrite lastResult/lastRunFinishedAt if
stopRequested was true (i.e., only set lastResult/lastRunFinishedAt when
stopRequested is false), then call await this.persistState(); keep references to
this.stateSnapshot and the completion/stop logic in the same method so the
semantics are unambiguous.
- Around line 382-415: The stop flow doesn't actually abort the running module
because runServiceInBackground/runBundledModuleWithRegistry aren't given an
AbortSignal and rely only on service.shouldStop; fix by creating an
AbortController when starting a run (in handleStartRequest), store its signal on
the instance (e.g. this.activeRunController and this.activeRunPromise), and pass
that AbortSignal into runServiceInBackground and onward into
runBundledModuleWithRegistry so the bundled runtime can be force-aborted; then
update handleStopRequest to call this.activeRunController?.abort() in addition
to flipping stopRequested/clearing alarms. Alternatively, if you prefer
cooperative cancellation only, add explicit documentation for the
service.shouldStop contract in package authoring docs, but the code-level fix is
to thread an AbortSignal through the symbols handleStartRequest,
handleStopRequest, runServiceInBackground, and runBundledModuleWithRegistry.
---
Nitpick comments:
In `@docs/use/execute.md`:
- Around line 108-112: Split the combined bullet into two distinct parts: one
bullet that explains storage scoping for "package service runs" (service-owned
durable state scoped to the declared service name) and a separate bullet or
subheading that describes the execution lifecycle semantics for service_start
(that service_start returns immediately and the service runs in the background
until finish/error/cooperative stop). Update the wording under the "package
service runs" statement to only cover storage concerns and create a new bullet
or a small subheading titled "Execution lifecycle" (or similar) to host the
service_start background/execution details so storage and lifecycle are clearly
separated.
- Around line 27-32: Update the docs to specify that service.setAlarm accepts a
Date object: document the parameter as setAlarm(runAt: Date) and note the
Promise return shape per ServiceToolOptions (Promise<{ ok: true; scheduled_at:
string }>) so users know to pass a JavaScript Date and what the method returns;
reference the implementation signature in ServiceToolOptions (see
run-codemode-registry.ts) when editing the list item for setAlarm.
In `@packages/worker/src/mcp/run-codemode-registry.node.test.ts`:
- Around line 926-939: Remove the inline assertion from the mockImplementation
of createExecuteExecutor (created as createExecuteExecutorSpy) and have the mock
simply return the fake executor; after the test's awaited call to
runBundledModuleWithRegistry, assert on the spy's call arguments instead (e.g.,
expect(createExecuteExecutorSpy).toHaveBeenCalledWith(expect.objectContaining({
timeoutMs: 300_000 }))). This keeps the mock free of side-effect assertions and
uses createExecuteExecutorSpy.mock.calls (or Jest/Vi matcher) to validate the
timeoutMs passed into createExecuteExecutor.
In `@packages/worker/src/package-runtime/package-service.ts`:
- Around line 463-480: alarm() currently always calls loadSavedPackageService
and then calls handleStartRequest which builds and discards an HTTP Response;
change it so alarm() catches failures from loadSavedPackageService and falls
back to this.stateSnapshot.autoStart (so auto-start intent isn't lost on
transient KV/D1 errors), and refactor the start flow by extracting the core
start logic from handleStartRequest into a new private method (e.g., private
executeStart(binding): Promise<PackageServiceRunResult | void>) that returns the
PackageServiceRunResult or void; have handleStartRequest call executeStart and
build its Response.json around that result, and have alarm() call executeStart
directly (no Response construction), while preserving the existing
persistState() call and stopRequested checks.
- Around line 439-461: The fetch method currently always calls await
request.json(), consuming the body before route matching; change fetch to first
inspect request.method and url.pathname and only parse the JSON body for the
POST routes that need it (e.g., before calling handleStartRequest,
handleStatusRequest, handleStopRequest). Keep the early binding check logic but
move the body parsing into the branches that call
handleStartRequest/handleStatusRequest/handleStopRequest; for the
/service-status branch call buildServiceStatusResponse(binding) without parsing
body, and return 404 for unmatched routes without touching request.json().
In `@packages/worker/src/repo/checks.ts`:
- Around line 201-210: The ambient declaration for
packageContext/serviceContext/service is too loose: update the `service` type
used in checks.ts to match the runtime contract provided by
`PackageServiceInstanceBase.runServiceInBackground` — make `setAlarm` accept
only `Date` (not string | Date) and tighten the return types of `getStatus`,
`setAlarm` and `clearAlarm` to the concrete shapes used at runtime (e.g., use
the `packageServiceStatusSchema` shape for `getStatus` and the actual `{ ok:
boolean; scheduled_at: string }`-like shape for `setAlarm`/`clearAlarm`), so
package authors get accurate IntelliSense and typechecking.
🪄 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: c1512569-4d25-40b3-82a9-ae73b2ede11b
📒 Files selected for processing (19)
docs/use/execute.mddocs/use/packages.mdpackages/worker/src/mcp/capabilities/services/service-get.tspackages/worker/src/mcp/capabilities/services/service-list.tspackages/worker/src/mcp/capabilities/services/service-start.tspackages/worker/src/mcp/capabilities/services/service-stop.tspackages/worker/src/mcp/capabilities/services/services-domain.node.test.tspackages/worker/src/mcp/capabilities/services/shared.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/run-codemode-registry.node.test.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/oauth-handlers.workers.test.tspackages/worker/src/package-registry/manifest.tspackages/worker/src/package-runtime/module-graph.tspackages/worker/src/package-runtime/package-app.tspackages/worker/src/package-runtime/package-service.node.test.tspackages/worker/src/package-runtime/package-service.tspackages/worker/src/package-runtime/published-runtime-artifacts.tspackages/worker/src/repo/checks.ts
✅ Files skipped from review due to trivial changes (2)
- docs/use/packages.md
- packages/worker/src/mcp/capabilities/services/services-domain.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/worker/src/package-runtime/package-service.node.test.ts
- packages/worker/src/mcp/capabilities/services/service-get.ts
- packages/worker/src/mcp/capabilities/services/service-list.ts
- packages/worker/src/package-runtime/package-app.ts
- packages/worker/src/mcp/run-codemode-registry.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| run_id: runId, | ||
| started_at: startedAt, | ||
| status: 'running', | ||
| } satisfies PackageServiceRunResult) |
Contributor
There was a problem hiding this comment.
Unnecessary alarm scheduled on first explicit start request
Low Severity
handleStartRequest calls initializeBinding with armAutoStart: true. On first binding when autoStart is true, initializeBinding schedules an immediate alarm (new Date()). Then handleStartRequest proceeds to start the service directly by setting currentRunId and calling runServiceInBackground. This means both an alarm AND a direct start are initiated. The alarm fires after the fetch returns, finds currentRunId set, and exits — but not before persisting a state write to clear alarm fields. This is a wasted DO storage write on every first start of an auto-start service.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cfe34b4. Configure here.
|
|
||
| export type PackageServiceStatusRecord = z.infer< | ||
| typeof packageServiceStatusSchema | ||
| > |
Contributor
There was a problem hiding this comment.
Duplicate type export across two modules
Low Severity
PackageServiceStatusRecord is independently exported from both package-service.ts and shared.ts as identically-defined z.infer type aliases of the same schema. shared.ts already re-exports the schema itself, so the second type alias is redundant and could confuse consumers about the canonical import location.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cfe34b4. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 597effc. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Apr 26, 2026
This was referenced May 8, 2026
Expose dynamic Workflows runtime in all execution contexts (hub Workflow + inline
code support)
#401
Merged
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
kody.servicespackage manifests, MCP service controls, and package-app service introspectionTesting
npm run typechecknpm run test -- packages/worker/src/package-registry/manifest.node.test.ts packages/worker/src/mcp/capabilities/services/services-domain.node.test.ts packages/worker/src/mcp/run-codemode-registry.node.test.ts packages/worker/src/package-runtime/package-service.node.test.ts packages/worker/src/package-runtime/module-graph.node.test.ts packages/worker/src/mcp/tools/search.node.test.ts packages/worker/src/repo/checks.node.test.ts packages/worker/src/oauth-handlers.workers.test.tsnpm run test -- packages/worker/src/mcp/capabilities/coding/kody-official-guide.node.test.ts packages/worker/src/package-registry/manifest.node.test.ts packages/worker/src/mcp/capabilities/services/services-domain.node.test.ts packages/worker/src/mcp/run-codemode-registry.node.test.ts packages/worker/src/package-runtime/package-service.node.test.ts packages/worker/src/package-runtime/module-graph.node.test.ts packages/worker/src/mcp/tools/search.node.test.ts packages/worker/src/repo/checks.node.test.ts packages/worker/src/oauth-handlers.workers.test.tsSummary by CodeRabbit
Release Notes
New Features
Documentation