Repository navigation
Audit capability and storage contracts - #628
Merged
Merged
Conversation
|
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 renames the codemode runtime and several built-in capability identifiers to capabilities-based names, threads capability source and remote-connector metadata through discovery and execution, validates and normalizes remote connector names, and switches repo/package manifest wiring to ChangesCapability runtime, naming, and storage migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Sandbox
participant Provider
participant RemoteProxy
participant Executor
participant ConnectorTool
Sandbox->>Provider: capabilities.remote["home"].set_pin(input)
Provider->>RemoteProxy: dispatch connector/capability and args
RemoteProxy->>Executor: callTool("remote:home:set_pin", args)
Executor->>ConnectorTool: invoke remote tool
ConnectorTool-->>Executor: result
Executor-->>Provider: result
Provider-->>Sandbox: result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
🔎 Preview deployed: https://kody-pr-628.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/worker/src/mcp/capabilities/build-capability-registry.ts (1)
85-92: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider O(n) duplicate detection.
entries.filter(... findIndex ...)is O(n²).createCapabilityMapruns at runtime when building per-caller remote-connector registries, where a connector can expose many tools, so this scales quadratically. ASetmakes it linear (and avoids listing the same name twice in the error message).♻️ Proposed linear duplicate check
- const entries = [ - ...primaryEntries, - ...aliasEntries.map(([name, capability]) => [name, capability] as const), - ] - const duplicates = entries.filter( - ([name], index) => - entries.findIndex(([entryName]) => entryName === name) !== index, - ) - if (duplicates.length > 0) { - const names = duplicates.map(([name]) => name).join(', ') - throw new Error(`Duplicate capability names or aliases: ${names}`) - } + const entries = [ + ...primaryEntries, + ...aliasEntries.map(([name, capability]) => [name, capability] as const), + ] + const seen = new Set<string>() + const duplicateNames = new Set<string>() + for (const [name] of entries) { + if (seen.has(name)) duplicateNames.add(name) + else seen.add(name) + } + if (duplicateNames.size > 0) { + throw new Error( + `Duplicate capability names or aliases: ${[...duplicateNames].join(', ')}`, + ) + }🤖 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/capabilities/build-capability-registry.ts` around lines 85 - 92, In createCapabilityMap, replace the current duplicates detection that uses entries.filter with nested findIndex because it is quadratic and can repeat the same name in the error list. Use a Set-based linear scan over entries to track seen names/aliases and collect only the first repeated capability names for the duplicate error, keeping the existing Error message shape in build-capability-registry.ts.packages/worker/src/mcp/capabilities/define-capability.ts (1)
71-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared logging fields.
Each
logMcpEventcall site now repeatscategory,tool,capabilityName,domain, andcapabilitySourceidentically across four branches. Factoring these into a shared base object would reduce duplication and the risk of the branches drifting apart as more fields are added.♻️ Suggested refactor
async handler(args, ctx) { const startedAt = performance.now() const { baseUrl, hasUser } = callerContextFields(ctx.callerContext) assertCallerCanAccessCapability(ctx.callerContext, definition) + const baseEvent = { + category: 'mcp' as const, + tool: 'capability' as const, + capabilityName: definition.name, + domain: definition.domain, + capabilitySource: source, + baseUrl, + hasUser, + } let parsedArgs: InferCapabilitySchema<TInputSchema> try { parsedArgs = inputParser(args) as InferCapabilitySchema<TInputSchema> } catch (error) { const { errorName, errorMessage } = errorFields(error) logMcpEvent({ - category: 'mcp', - tool: 'capability', - capabilityName: definition.name, - domain: definition.domain, - capabilitySource: source, + ...baseEvent, outcome: 'failure', durationMs: Math.round(performance.now() - startedAt), - baseUrl, - hasUser, failurePhase: 'parse_input', errorName, errorMessage, cause: error, }) throw error } // similarly for the remaining three call sites🤖 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/capabilities/define-capability.ts` around lines 71 - 155, The handler in defineCapability repeats the same logMcpEvent metadata across the parse_input, handler, parse_output, and success branches. Extract the shared fields (category, tool, capabilityName, domain, and capabilitySource) into a common base object inside async handler and spread it into each logMcpEvent call, keeping only the branch-specific fields alongside it to avoid duplication and future drift.packages/worker/src/mcp/capabilities/meta/meta-list-capabilities.node.test.ts (1)
166-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing test for the actual bug this PR fixes:
remote:*domain filtering.This new test only covers filtering by a builtin domain (
'packages'). The PR's stated motivation is fixingmeta_list_capabilitiesdomain filtering for synthesizedremote:*domains (e.g.remote:roku:default), but no test asserts that adomain: 'remote:roku:default'filter actually returns thepressKeyCapabilityand excludes builtin capabilities. Given the domain schema was loosened specifically to support this case, a direct regression test would be valuable.✅ Suggested additional test
+ const remoteConnectorOnly = await metaListCapabilitiesCapability.handler( + { + domain: 'remote:roku:default', + }, + { + env: buildRemoteConnectorEnv(), + callerContext: createMcpCallerContext({ + baseUrl: 'https://heykody.dev', + }), + }, + ) + + expect(remoteConnectorOnly.total).toBeGreaterThan(0) + expect( + remoteConnectorOnly.capabilities.every( + (capability) => capability.domain === 'remote:roku:default', + ), + ).toBe(true)🤖 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/capabilities/meta/meta-list-capabilities.node.test.ts` around lines 166 - 184, Add a regression test for the actual remote domain filtering bug in metaListCapabilitiesCapability.handler, not just the builtin packages case. In meta-list-capabilities.node.test.ts, extend the existing coverage to call the handler with domain set to a synthesized remote value such as remote:roku:default and assert it returns the remote capability (for example pressKeyCapability) while excluding builtin capabilities. Keep the new assertion alongside the current packages-only test so the behavior of remote:* domain matching is explicitly verified.
🤖 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/capabilities/build-capability-registry.ts`:
- Around line 85-92: In createCapabilityMap, replace the current duplicates
detection that uses entries.filter with nested findIndex because it is quadratic
and can repeat the same name in the error list. Use a Set-based linear scan over
entries to track seen names/aliases and collect only the first repeated
capability names for the duplicate error, keeping the existing Error message
shape in build-capability-registry.ts.
In `@packages/worker/src/mcp/capabilities/define-capability.ts`:
- Around line 71-155: The handler in defineCapability repeats the same
logMcpEvent metadata across the parse_input, handler, parse_output, and success
branches. Extract the shared fields (category, tool, capabilityName, domain, and
capabilitySource) into a common base object inside async handler and spread it
into each logMcpEvent call, keeping only the branch-specific fields alongside it
to avoid duplication and future drift.
In
`@packages/worker/src/mcp/capabilities/meta/meta-list-capabilities.node.test.ts`:
- Around line 166-184: Add a regression test for the actual remote domain
filtering bug in metaListCapabilitiesCapability.handler, not just the builtin
packages case. In meta-list-capabilities.node.test.ts, extend the existing
coverage to call the handler with domain set to a synthesized remote value such
as remote:roku:default and assert it returns the remote capability (for example
pressKeyCapability) while excluding builtin capabilities. Keep the new assertion
alongside the current packages-only test so the behavior of remote:* domain
matching is explicitly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e79d8ca-62ba-4694-9760-b233fae00f6d
📒 Files selected for processing (22)
docs/contributing/adding-capabilities.mddocs/contributing/architecture/capability-and-primitive-audit-2026-07.mddocs/contributing/architecture/data-storage.mddocs/contributing/architecture/primitives.yamlpackages/worker/src/mcp/capabilities/access-control.tspackages/worker/src/mcp/capabilities/build-capability-registry.tspackages/worker/src/mcp/capabilities/build-capability-registry.workers.test.tspackages/worker/src/mcp/capabilities/capability-search.tspackages/worker/src/mcp/capabilities/capability-search.workers.test.tspackages/worker/src/mcp/capabilities/define-capability.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.node.test.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.tspackages/worker/src/mcp/capabilities/registry.tspackages/worker/src/mcp/capabilities/remote-connector/index.tspackages/worker/src/mcp/capabilities/types.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/observability.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.ts
…8b' into audit-capability-primitive-contracts # Conflicts: # docs/contributing/adding-capabilities.md # docs/contributing/architecture/capability-and-primitive-audit-2026-07.md # packages/worker/src/mcp/capabilities/build-capability-registry.ts # packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts # packages/worker/src/mcp/capabilities/capability-search.workers.test.ts # packages/worker/src/mcp/capabilities/define-capability.ts # packages/worker/src/mcp/capabilities/meta/meta-list-capabilities.node.test.ts # packages/worker/src/mcp/capabilities/meta/meta-list-capabilities.ts # packages/worker/src/mcp/capabilities/remote-connector/index.ts # packages/worker/src/mcp/capabilities/types.ts # packages/worker/src/mcp/tools/search-format.node.test.ts # packages/worker/src/mcp/tools/search-format.ts
| month.usage, | ||
| metric as AdminUsageRollup['metric'], | ||
| )?.eventCount ?? 0, | ||
| )} |
Contributor
There was a problem hiding this comment.
Wrong service runtime month metric
Medium Severity
In the admin usage drill-down, month-over-month rows label the service_runtime column like the summary table but render eventCount for every metric. service_runtime usage is tracked as accumulated duration (totalDurationMs), so the history table can show run counts while the overview shows time, misleading quota review.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c224247. Configure here.
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 (1)
packages/worker/src/mcp/tools/search-format.ts (1)
811-834: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMisleading "Built-in capabilities" wording for remote-connector detail pages.
The static line
'Built-in capabilities returned by \search` are available inside `execute` as methods on the imported `codemode` object.'is shown unconditionally, even for remote-connector capabilities (as demonstrated by the newset_pintest). This contradicts both the fact that these are not "built-in" and the code example immediately below it, which correctly showscodemode.remote["home/default"].set_pin(input)rather than a flat method call.search.ts's guidance text already branches onsource === 'remote-connector'` (context snippet 3); this markdown generator should do the same to avoid confusing callers/agents about how to invoke the capability.📝 Suggested fix
'## Execute from `execute`', '', - 'Built-in capabilities returned by `search` are available inside `execute` as methods on the imported `codemode` object.', + detail.spec.source === 'remote-connector' + ? 'Remote-connector capabilities returned by `search` are available inside `execute` via `codemode.remote[connectorName]`.' + : 'Built-in capabilities returned by `search` are available inside `execute` as methods on the imported `codemode` object.', '',🤖 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/tools/search-format.ts` around lines 811 - 834, The capability detail markdown in formatEntityDetailMarkdown always prints built-in execution guidance, which is misleading for remote-connector capabilities. Update the message generation to branch on detail.spec.source like search.ts does, so remote-connector entries use remote invocation wording and built-in entries keep the current codemode guidance. Keep the example text consistent with the source-specific call style shown by buildCapabilityExecuteExample and the surrounding execute section.
♻️ Duplicate comments (1)
packages/worker/src/mcp/run-codemode-registry.ts (1)
59-92: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCross-reference: see duplicate-type comment on
packages/worker/src/mcp/executor.ts(Lines 118-133) — this exportedCodemodeRemoteConnectorMetadata/KodyResolvedProviderpair is redefined there with a narrower shape.🤖 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 59 - 92, The exported CodemodeRemoteConnectorMetadata and KodyResolvedProvider types are duplicated with a narrower shape in executor.ts, so make both modules share the same source of truth or align the definitions. Update the types around CodemodeRemoteConnectorMetadata, CodemodeRemoteCapabilityMetadata, and KodyResolvedProvider so executor.ts does not redefine a conflicting subset and instead imports or reuses the wider metadata shape from run-codemode-registry.ts.
🧹 Nitpick comments (7)
packages/worker/src/mcp/tools/search-format.ts (1)
444-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated accessor-generation logic vs.
search.ts.
buildCodemodeCapabilityAccessorreimplements the same identifier-regex +codemode.remote[connector].toolconstruction already present insearch.ts(thetopMatch.source === 'remote-connector'branch building the guidance accessor). If escaping/identifier rules ever change, one copy can drift from the other, producing inconsistent guidance text vs. generated code examples for remote-connector capabilities.Consider extracting a single shared helper (e.g. in a small util module) that both
search.tsandsearch-format.tsimport.♻️ Suggested extraction
+// shared util, e.g. packages/worker/src/mcp/tools/codemode-accessor.ts +export function buildRemoteCodemodeAccessor(connectorName: string, toolName: string) { + const connectorAccessor = `codemode.remote[${JSON.stringify(connectorName)}]` + return /^[A-Za-z_$][\w$]*$/.test(toolName) + ? `${connectorAccessor}.${toolName}` + : `${connectorAccessor}[${JSON.stringify(toolName)}]` +}🤖 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/tools/search-format.ts` around lines 444 - 478, The accessor-building logic is duplicated between buildCodemodeCapabilityAccessor in search-format.ts and the corresponding remote-connector accessor construction in search.ts, so any future change to identifier escaping or connector/tool formatting could drift. Extract the shared accessor-generation into a single helper and have both call sites use it, keeping the codemode.remote[connector].tool and fallback identifier handling consistent across search guidance and example generation.packages/worker/client/routes/admin-usage.tsx (1)
119-159: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueClient-side refetch doesn't cancel prior in-flight requests.
loadAdminUsagedoesn't pass anAbortSignaltofetch(unlikeadminUsageRouteLoader, which accepts one). TherequestIdguard prevents stale responses from being applied, but the older request still completes on the network/worker, wasting resources on rapid navigation.🤖 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/client/routes/admin-usage.tsx` around lines 119 - 159, The client-side refetch in loadAdminUsage still leaves previous requests running, since fetch is not wired to an AbortSignal. Update loadAdminUsage to create/track an AbortController, pass its signal into the fetch call, and abort the prior in-flight request when a newer request starts; keep the existing requestId guard and apply the same cancellation pattern used by adminUsageRouteLoader/readJson handling.packages/worker/src/mcp/capabilities/admin/admin-usage-overview.ts (1)
127-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRound-tripping args through a synthetic URL to reuse
loadAdminUsageData.The handler builds a fake
URLobject solely to serializepage/pageSize/userIdback into query params soloadAdminUsageDatacan re-parse them fromrequestUrl: string. Since this capability already receives typed/validated args frominputSchema, this indirection (serialize → re-parse) is unnecessary complexity. A typed overload/params-object variant ofloadAdminUsageDatawould remove the redundant marshaling in both this file and the SSR/API handler inadmin-usage.ts.🤖 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/capabilities/admin/admin-usage-overview.ts` around lines 127 - 145, The handler in admin_usage_overview is unnecessarily serializing typed args into a synthetic URL just to call loadAdminUsageData and let it re-parse them. Refactor loadAdminUsageData to accept a typed params object or add an overload, then update admin_usage_overview.handler to pass args directly instead of constructing a URL. Also adjust the existing admin-usage.ts caller to use the same typed path so both call sites avoid the serialize/re-parse round-trip.packages/worker/src/app/handlers/admin-usage.ts (1)
9-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant session re-check after role enforcement.
requireUserWithRolealready throws (as aResponse) when the caller is unauthenticated or lacks the admin role (perrequireAuthorizedUser/requireUserWithRolecontract). The subsequentreadAuthSessionResult/!sessionredirect at Lines 20-23 is then either dead code (ifrequireUserWithRolealways covers "no session") or, if reachable, indicates the two checks are using different auth sources — worth confirming which is actually the case.♻️ Proposed simplification (if session check is dead code)
async handler({ request }) { try { await requireUserWithRole(request, env, 'admin') } catch (error) { if (error instanceof Response) return error throw error } - const { session } = await readAuthSessionResult(request) - if (!session) { - return redirectToLogin(request) - } - const adminUsage = await loadAdminUsageData(env, request.url)🤖 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/app/handlers/admin-usage.ts` around lines 9 - 23, The session re-check in createAdminUsageHandler is redundant after requireUserWithRole already enforces authentication and admin access via Response throws. Remove the readAuthSessionResult/request !session redirect path if requireUserWithRole fully covers unauthenticated users, and keep the handler focused on the requireUserWithRole guard and the subsequent admin logic.packages/worker/src/mcp/capabilities/secrets/jwt-sign.ts (1)
90-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated capability-name literal to a constant.
'secret_jwt_sign'is hardcoded three separate times (allow-list check, approval URL, denial message) plus once more in thenamefield. Any future rename of this capability risks drift between these literals — which is precisely the class of bug this PR's audit is meant to eliminate.♻️ Proposed fix
+const JWT_SIGN_CAPABILITY_NAME = 'secret_jwt_sign' + export const jwtSignCapability = defineDomainCapability( capabilityDomainNames.secrets, { - name: 'secret_jwt_sign', + name: JWT_SIGN_CAPABILITY_NAME, ... - if (!resolved.allowedCapabilities.includes('secret_jwt_sign')) { + if (!resolved.allowedCapabilities.includes(JWT_SIGN_CAPABILITY_NAME)) { const approvalUrl = buildSecretCapabilityApprovalUrl({ baseUrl: ctx.callerContext.baseUrl, name: args.private_key_secret_name, scope: resolved.scope ?? args.private_key_secret_scope ?? 'user', - capabilityName: 'secret_jwt_sign', + capabilityName: JWT_SIGN_CAPABILITY_NAME, storageContext, }) throw new Error( createCapabilitySecretAccessDeniedMessage( args.private_key_secret_name, - 'secret_jwt_sign', + JWT_SIGN_CAPABILITY_NAME, approvalUrl, ), ) }🤖 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/capabilities/secrets/jwt-sign.ts` around lines 90 - 105, The capability name is duplicated as a hardcoded literal in the JWT secret signing flow, which can drift if renamed. Introduce a single shared constant in the jwt-sign capability module and use it in the allowedCapabilities check, buildSecretCapabilityApprovalUrl call, createCapabilitySecretAccessDeniedMessage call, and any related name field so all references stay aligned.packages/worker/src/app/loader-data.ts (1)
116-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate plan-name union instead of reusing
PlanName.
AdminUsagePlanNamere-declares the same literal set thatPlanName(from#worker/entitlements/plans.ts) already defines. If the canonical plan list changes there, this type silently drifts instead of failing to compile.♻️ Suggested fix
-export type AdminUsagePlanName = 'partner' | 'personal' | 'pro' +export type { PlanName as AdminUsagePlanName } from '`#worker/entitlements/plans.ts`'🤖 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/app/loader-data.ts` at line 116, AdminUsagePlanName is duplicating the plan literal union already defined by PlanName, so update loader-data to reuse the canonical type from `#worker/entitlements/plans.ts` instead of redeclaring the literals. Adjust the AdminUsagePlanName definition and any related imports/usages so the admin usage plan type is derived from PlanName and stays in sync automatically.packages/worker/src/app/admin-usage-data.ts (1)
344-356: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded per-user history query.
loadUserMonthRollupsfetches every historicalusage_rollupsrow for a user with noLIMIT/date filter, relying ontoMonthUsageto sort and slice to the last 12 months in application code. As history accumulates this row set (and the memory/CPU cost of sorting it) grows unbounded per selected-user request. Consider filtering by a bounded date range (e.g., last ~13 months) or addingLIMITin SQL.🤖 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/app/admin-usage-data.ts` around lines 344 - 356, `loadUserMonthRollups` currently pulls an unbounded history for each user, which makes selected-user requests grow in memory and CPU over time. Update the SQL in `loadUserMonthRollups` to constrain the `usage_rollups` rows returned, either by adding a bounded month/date filter (preferably around the last 12–13 months) or an appropriate `LIMIT`, and keep `toMonthUsage` as a formatting step rather than the place where the dataset is reduced.
🤖 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/client/routes/admin-usage.tsx`:
- Around line 502-516: The month-over-month usage table in admin-usage.tsx
renders service_runtime incorrectly as an event count via the
selectedUser.monthUsage map and getMetric lookup, while the current-month table
shows it as a duration. Update the month-over-month rendering logic so the
service_runtime column uses the same duration formatting as the current-month
view, while leaving the other metrics in the table unchanged. Use the existing
usageMetricLabels, getMetric, and formatDuration/formatInteger paths to keep the
display consistent across both panels.
In `@packages/worker/src/app/admin-usage-data.ts`:
- Around line 256-323: The per-user resource readers are causing an N+1 query
fan-out in the admin usage page. Update `readDailyCounters`,
`readResourceCounts`, and `readEntitlementConsumption` to batch
`readEntitlementResourceUsage` lookups across users/resources instead of
querying once per user per resource, using the same pattern as
`loadCurrentMonthRollups` with an `IN (...)` style batch query. Keep the
existing return shapes in `AdminUsageDailyCounter`, `AdminUsageResourceCount`,
and `AdminUsageEntitlementConsumption`, but populate them from the batched
results rather than individual calls.
- Around line 218-254: buildSelectedUser is recomputing the selected user
summary and also blocking the intended parallel I/O. Update buildSelectedUser to
reuse the matching summary already produced by loadAdminUsageData/summaries when
the selected user is present, rather than calling buildUserSummary again, so
todayCounters/resourceCounts/current-month usage aren’t fetched twice. Also
remove the nested await from the Promise.all setup and start
loadCurrentMonthRollups, loadUserMonthRollups, and readEntitlementConsumption as
separate promises so they run concurrently.
- Around line 294-323: `readEntitlementConsumption` is treating a zero limit as
if the resource were unlimited, which hides plan violations. Update the
`percentOfLimit`/`overEightyPercent` logic in `readEntitlementConsumption` so
that `limit === 0` is handled as a hard cap (not null), and only `null` means
unlimited; keep using `resolvePlanLimit` and the returned `current` value to
surface warnings when usage is non-zero against a zero limit.
In `@packages/worker/src/mcp/executor.ts`:
- Around line 118-133: The duplicated CodemodeRemoteConnectorMetadata and
KodyResolvedProvider definitions in executor.ts should be removed and replaced
with imports from the single source of truth used by
buildCodemodeRemoteConnectorMetadata in run-codemode-registry.ts. Update
executor.ts to consume the shared exported types (or move both types into a
dedicated shared module) so the metadata shape, including fields like kind,
instanceId, and status properties, stays consistent across files and cannot
diverge silently.
- Around line 399-444: Add an end-to-end test for the generated codemode proxy
source by exercising createCodemodeProviderProxySource, not just
createCodemodeRemoteProxy directly. Build a realistic
ResolvedProvider/KodyResolvedProvider input with kodyRemoteConnectors and assert
the emitted string includes the expected __kodyCreateCodemodeRemoteProxy, remote
access, and dispatch behavior. Use createCodemodeProviderProxySource and
createCodemodeRemoteProxy as the key symbols to locate and validate the eval’d
worker path.
In `@packages/worker/src/remote-connector/remote-domain-id.ts`:
- Around line 3-31: The generated names from remoteConnectorInstanceSlug and
remoteConnectorToolName can still collide after slugification, causing
createCapabilityMap to reject duplicate registrations. Update the name
generation logic in remoteConnectorInstanceSlug and remoteConnectorToolName (and
any callers like remoteConnectorDomainId/remoteConnectorCodemodeName if they
reuse them) to append a stable disambiguator derived from the original input,
such as a short hash, while keeping the readable slug prefix. Preserve
determinism so the same source string always maps to the same final capability
name.
---
Outside diff comments:
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Around line 811-834: The capability detail markdown in
formatEntityDetailMarkdown always prints built-in execution guidance, which is
misleading for remote-connector capabilities. Update the message generation to
branch on detail.spec.source like search.ts does, so remote-connector entries
use remote invocation wording and built-in entries keep the current codemode
guidance. Keep the example text consistent with the source-specific call style
shown by buildCapabilityExecuteExample and the surrounding execute section.
---
Duplicate comments:
In `@packages/worker/src/mcp/run-codemode-registry.ts`:
- Around line 59-92: The exported CodemodeRemoteConnectorMetadata and
KodyResolvedProvider types are duplicated with a narrower shape in executor.ts,
so make both modules share the same source of truth or align the definitions.
Update the types around CodemodeRemoteConnectorMetadata,
CodemodeRemoteCapabilityMetadata, and KodyResolvedProvider so executor.ts does
not redefine a conflicting subset and instead imports or reuses the wider
metadata shape from run-codemode-registry.ts.
---
Nitpick comments:
In `@packages/worker/client/routes/admin-usage.tsx`:
- Around line 119-159: The client-side refetch in loadAdminUsage still leaves
previous requests running, since fetch is not wired to an AbortSignal. Update
loadAdminUsage to create/track an AbortController, pass its signal into the
fetch call, and abort the prior in-flight request when a newer request starts;
keep the existing requestId guard and apply the same cancellation pattern used
by adminUsageRouteLoader/readJson handling.
In `@packages/worker/src/app/admin-usage-data.ts`:
- Around line 344-356: `loadUserMonthRollups` currently pulls an unbounded
history for each user, which makes selected-user requests grow in memory and CPU
over time. Update the SQL in `loadUserMonthRollups` to constrain the
`usage_rollups` rows returned, either by adding a bounded month/date filter
(preferably around the last 12–13 months) or an appropriate `LIMIT`, and keep
`toMonthUsage` as a formatting step rather than the place where the dataset is
reduced.
In `@packages/worker/src/app/handlers/admin-usage.ts`:
- Around line 9-23: The session re-check in createAdminUsageHandler is redundant
after requireUserWithRole already enforces authentication and admin access via
Response throws. Remove the readAuthSessionResult/request !session redirect path
if requireUserWithRole fully covers unauthenticated users, and keep the handler
focused on the requireUserWithRole guard and the subsequent admin logic.
In `@packages/worker/src/app/loader-data.ts`:
- Line 116: AdminUsagePlanName is duplicating the plan literal union already
defined by PlanName, so update loader-data to reuse the canonical type from
`#worker/entitlements/plans.ts` instead of redeclaring the literals. Adjust the
AdminUsagePlanName definition and any related imports/usages so the admin usage
plan type is derived from PlanName and stays in sync automatically.
In `@packages/worker/src/mcp/capabilities/admin/admin-usage-overview.ts`:
- Around line 127-145: The handler in admin_usage_overview is unnecessarily
serializing typed args into a synthetic URL just to call loadAdminUsageData and
let it re-parse them. Refactor loadAdminUsageData to accept a typed params
object or add an overload, then update admin_usage_overview.handler to pass args
directly instead of constructing a URL. Also adjust the existing admin-usage.ts
caller to use the same typed path so both call sites avoid the
serialize/re-parse round-trip.
In `@packages/worker/src/mcp/capabilities/secrets/jwt-sign.ts`:
- Around line 90-105: The capability name is duplicated as a hardcoded literal
in the JWT secret signing flow, which can drift if renamed. Introduce a single
shared constant in the jwt-sign capability module and use it in the
allowedCapabilities check, buildSecretCapabilityApprovalUrl call,
createCapabilitySecretAccessDeniedMessage call, and any related name field so
all references stay aligned.
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Around line 444-478: The accessor-building logic is duplicated between
buildCodemodeCapabilityAccessor in search-format.ts and the corresponding
remote-connector accessor construction in search.ts, so any future change to
identifier escaping or connector/tool formatting could drift. Extract the shared
accessor-generation into a single helper and have both call sites use it,
keeping the codemode.remote[connector].tool and fallback identifier handling
consistent across search guidance and example generation.
🪄 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: 6d506241-0179-4542-945f-22090c0e7de0
📒 Files selected for processing (64)
docs/contributing/adding-capabilities.mddocs/contributing/architecture/capability-and-primitive-audit-2026-07.mddocs/contributing/architecture/remote-connectors.mddocs/contributing/architecture/request-lifecycle.mddocs/contributing/package-invocation-api.mddocs/contributing/packages-and-manifests.mddocs/guides/README.mddocs/guides/account-secret-setup.mddocs/guides/integration-bootstrap.mddocs/guides/package-subscriptions.mddocs/use/email-primitives.mddocs/use/first-steps.mddocs/use/packages.mddocs/use/search.mddocs/use/secrets-and-values.mddocs/use/workflows.mde2e/admin-rbac.spec.tspackages/worker/client/routes/admin-community-reports.tsxpackages/worker/client/routes/admin-invites.tsxpackages/worker/client/routes/admin-roles.tsxpackages/worker/client/routes/admin-usage.tsxpackages/worker/client/routes/admin-users.tsxpackages/worker/client/routes/index.tsxpackages/worker/src/app/admin-usage-data.node.test.tspackages/worker/src/app/admin-usage-data.tspackages/worker/src/app/handlers/admin-usage.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/router.tspackages/worker/src/app/routes.tspackages/worker/src/entitlements/errors.tspackages/worker/src/entitlements/service.tspackages/worker/src/mcp/capabilities/access-control.tspackages/worker/src/mcp/capabilities/admin/admin-capabilities.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-usage-overview.tspackages/worker/src/mcp/capabilities/admin/domain.tspackages/worker/src/mcp/capabilities/build-capability-registry.tspackages/worker/src/mcp/capabilities/build-capability-registry.workers.test.tspackages/worker/src/mcp/capabilities/capability-search.workers.test.tspackages/worker/src/mcp/capabilities/coding/kody-official-guide.node.test.tspackages/worker/src/mcp/capabilities/coding/kody-official-guide.tspackages/worker/src/mcp/capabilities/define-capability.tspackages/worker/src/mcp/capabilities/jobs/workflow-list.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.node.test.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.tspackages/worker/src/mcp/capabilities/packages/list-package-subscriptions.tspackages/worker/src/mcp/capabilities/remote-connector/index.tspackages/worker/src/mcp/capabilities/secrets/jwt-sign.node.test.tspackages/worker/src/mcp/capabilities/secrets/jwt-sign.tspackages/worker/src/mcp/capabilities/types.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/run-codemode-registry.node.test.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/mcp/server-instructions.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-runtime/package-app.node.test.tspackages/worker/src/package-runtime/package-app.tspackages/worker/src/package-runtime/package-workflows.node.test.tspackages/worker/src/remote-connector/remote-domain-id.ts
💤 Files with no reviewable changes (2)
- packages/worker/src/mcp/capabilities/capability-search.workers.test.ts
- packages/worker/src/mcp/capabilities/define-capability.ts
✅ Files skipped from review due to trivial changes (19)
- packages/worker/src/mcp/capabilities/admin/domain.ts
- docs/use/workflows.md
- packages/worker/client/routes/admin-roles.tsx
- docs/contributing/package-invocation-api.md
- packages/worker/client/routes/admin-invites.tsx
- packages/worker/client/routes/admin-users.tsx
- docs/guides/README.md
- packages/worker/client/routes/admin-community-reports.tsx
- packages/worker/src/mcp/capabilities/coding/kody-official-guide.node.test.ts
- docs/contributing/architecture/remote-connectors.md
- packages/worker/src/mcp/tools/open-generated-ui.ts
- docs/use/first-steps.md
- docs/guides/integration-bootstrap.md
- docs/guides/package-subscriptions.md
- docs/contributing/architecture/capability-and-primitive-audit-2026-07.md
- docs/contributing/adding-capabilities.md
- docs/contributing/architecture/request-lifecycle.md
- docs/use/search.md
- packages/worker/src/mcp/tools/execute.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/executor.node.test.ts
Comment on lines
+502
to
+516
| {selectedUser.monthUsage.map((month) => ( | ||
| <tr key={month.month}> | ||
| <td mix={css(cellCss)}>{month.month}</td> | ||
| {Object.keys(usageMetricLabels).map((metric) => ( | ||
| <td key={metric} mix={css(numericCellCss)}> | ||
| {formatInteger( | ||
| getMetric( | ||
| month.usage, | ||
| metric as AdminUsageRollup['metric'], | ||
| )?.eventCount ?? 0, | ||
| )} | ||
| </td> | ||
| ))} | ||
| </tr> | ||
| ))} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Service runtime shown as event count in month-over-month table, inconsistent with current-month table.
In the current-month table, service_runtime is rendered with formatDuration(serviceRuntime?.totalDurationMs ?? 0) (Lines 345-349), but the month-over-month table renders every metric — including service_runtime — via formatInteger(getMetric(...)?.eventCount ?? 0). This means the same "Service runtime" column shows a duration in one panel and a raw event count in the other, which is misleading for admins comparing usage across the two views.
🐛 Proposed fix
{selectedUser.monthUsage.map((month) => (
<tr key={month.month}>
<td mix={css(cellCss)}>{month.month}</td>
- {Object.keys(usageMetricLabels).map((metric) => (
- <td key={metric} mix={css(numericCellCss)}>
- {formatInteger(
- getMetric(
- month.usage,
- metric as AdminUsageRollup['metric'],
- )?.eventCount ?? 0,
- )}
- </td>
- ))}
+ {Object.keys(usageMetricLabels).map((metric) => {
+ const entry = getMetric(
+ month.usage,
+ metric as AdminUsageRollup['metric'],
+ )
+ return (
+ <td key={metric} mix={css(numericCellCss)}>
+ {metric === 'service_runtime'
+ ? formatDuration(entry?.totalDurationMs ?? 0)
+ : formatInteger(entry?.eventCount ?? 0)}
+ </td>
+ )
+ })}
</tr>
))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {selectedUser.monthUsage.map((month) => ( | |
| <tr key={month.month}> | |
| <td mix={css(cellCss)}>{month.month}</td> | |
| {Object.keys(usageMetricLabels).map((metric) => ( | |
| <td key={metric} mix={css(numericCellCss)}> | |
| {formatInteger( | |
| getMetric( | |
| month.usage, | |
| metric as AdminUsageRollup['metric'], | |
| )?.eventCount ?? 0, | |
| )} | |
| </td> | |
| ))} | |
| </tr> | |
| ))} | |
| {selectedUser.monthUsage.map((month) => ( | |
| <tr key={month.month}> | |
| <td mix={css(cellCss)}>{month.month}</td> | |
| {Object.keys(usageMetricLabels).map((metric) => { | |
| const entry = getMetric( | |
| month.usage, | |
| metric as AdminUsageRollup['metric'], | |
| ) | |
| return ( | |
| <td key={metric} mix={css(numericCellCss)}> | |
| {metric === 'service_runtime' | |
| ? formatDuration(entry?.totalDurationMs ?? 0) | |
| : formatInteger(entry?.eventCount ?? 0)} | |
| </td> | |
| ) | |
| })} | |
| </tr> | |
| ))} |
🤖 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/client/routes/admin-usage.tsx` around lines 502 - 516, The
month-over-month usage table in admin-usage.tsx renders service_runtime
incorrectly as an event count via the selectedUser.monthUsage map and getMetric
lookup, while the current-month table shows it as a duration. Update the
month-over-month rendering logic so the service_runtime column uses the same
duration formatting as the current-month view, while leaving the other metrics
in the table unchanged. Use the existing usageMetricLabels, getMetric, and
formatDuration/formatInteger paths to keep the display consistent across both
panels.
Comment on lines
+218
to
+254
| async function buildSelectedUser(input: { | ||
| db: D1Database | ||
| user: UserWithUsageId | ||
| currentMonth: string | ||
| now: Date | ||
| }): Promise<AdminUsageSelectedUser> { | ||
| const [summary, monthRows, entitlementConsumption] = await Promise.all([ | ||
| buildUserSummary({ | ||
| db: input.db, | ||
| user: input.user, | ||
| usage: await loadCurrentMonthRollups({ | ||
| db: input.db, | ||
| userIds: [input.user.usageUserId], | ||
| month: input.currentMonth, | ||
| }), | ||
| now: input.now, | ||
| }), | ||
| loadUserMonthRollups({ | ||
| db: input.db, | ||
| userId: input.user.usageUserId, | ||
| }), | ||
| readEntitlementConsumption({ | ||
| db: input.db, | ||
| user: input.user, | ||
| now: input.now, | ||
| }), | ||
| ]) | ||
| const warnings = entitlementConsumption.filter( | ||
| (item) => item.overEightyPercent, | ||
| ) | ||
| return { | ||
| ...summary, | ||
| monthUsage: toMonthUsage(monthRows, input.currentMonth), | ||
| entitlementConsumption, | ||
| warnings, | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
buildSelectedUser re-does work already computed for the page, and serializes calls that should run in parallel.
Two issues here:
- Duplicate queries: the selected user's
todayCounters/resourceCounts/current-month usage are frequently already computed in thesummariesarray built inloadAdminUsageData(lines 132-141) when the selected user is one of the paginated rows (the common case, e.g. defaultselectedUserId=0).buildSelectedUserrecomputes all of this from scratch via a freshbuildUserSummarycall instead of reusing the existing summary. - Broken parallelism:
await loadCurrentMonthRollups(...)is nested inside the first element of thePromise.all([...])array literal (lines 228-234). Because array elements are evaluated eagerly beforePromise.allis invoked, thisawaitsuspends evaluation of the whole array, soloadUserMonthRollupsandreadEntitlementConsumption(lines 235-244) don't even start untilloadCurrentMonthRollupsresolves — turning what looks like 3-way parallel I/O into a partially-serial chain.
♻️ Suggested fix
async function buildSelectedUser(input: {
db: D1Database
user: UserWithUsageId
currentMonth: string
now: Date
}): Promise<AdminUsageSelectedUser> {
- const [summary, monthRows, entitlementConsumption] = await Promise.all([
- buildUserSummary({
- db: input.db,
- user: input.user,
- usage: await loadCurrentMonthRollups({
- db: input.db,
- userIds: [input.user.usageUserId],
- month: input.currentMonth,
- }),
- now: input.now,
- }),
- loadUserMonthRollups({
- db: input.db,
- userId: input.user.usageUserId,
- }),
- readEntitlementConsumption({
- db: input.db,
- user: input.user,
- now: input.now,
- }),
- ])
+ const [currentMonthRows, monthRows, entitlementConsumption, summary] =
+ await Promise.all([
+ loadCurrentMonthRollups({
+ db: input.db,
+ userIds: [input.user.usageUserId],
+ month: input.currentMonth,
+ }),
+ loadUserMonthRollups({ db: input.db, userId: input.user.usageUserId }),
+ readEntitlementConsumption({
+ db: input.db,
+ user: input.user,
+ now: input.now,
+ }),
+ buildUserSummary({
+ db: input.db,
+ user: input.user,
+ usage: [],
+ now: input.now,
+ }),
+ ])Note: reusing an already-computed page-level summary for the selected user (instead of recomputing it here) would additionally avoid duplicating readDailyCounters/readResourceCounts work.
🤖 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/app/admin-usage-data.ts` around lines 218 - 254,
buildSelectedUser is recomputing the selected user summary and also blocking the
intended parallel I/O. Update buildSelectedUser to reuse the matching summary
already produced by loadAdminUsageData/summaries when the selected user is
present, rather than calling buildUserSummary again, so
todayCounters/resourceCounts/current-month usage aren’t fetched twice. Also
remove the nested await from the Promise.all setup and start
loadCurrentMonthRollups, loadUserMonthRollups, and readEntitlementConsumption as
separate promises so they run concurrently.
Comment on lines
+256
to
+323
| async function readDailyCounters(input: { | ||
| db: D1Database | ||
| user: UserWithUsageId | ||
| now: Date | ||
| }): Promise<Array<AdminUsageDailyCounter>> { | ||
| return await Promise.all( | ||
| adminUsageDailyCounterResources.map(async (resource) => ({ | ||
| resource, | ||
| label: entitlementResourceLabels[resource], | ||
| count: await readEntitlementResourceUsage({ | ||
| db: input.db, | ||
| userId: input.user.usageUserId, | ||
| resource, | ||
| now: input.now, | ||
| }), | ||
| })), | ||
| ) | ||
| } | ||
|
|
||
| async function readResourceCounts(input: { | ||
| db: D1Database | ||
| user: UserWithUsageId | ||
| now: Date | ||
| }): Promise<Array<AdminUsageResourceCount>> { | ||
| return await Promise.all( | ||
| adminUsageLiveResourceCounts.map(async (resource) => ({ | ||
| resource, | ||
| label: entitlementResourceLabels[resource], | ||
| current: await readEntitlementResourceUsage({ | ||
| db: input.db, | ||
| userId: input.user.usageUserId, | ||
| resource, | ||
| now: input.now, | ||
| }), | ||
| })), | ||
| ) | ||
| } | ||
|
|
||
| async function readEntitlementConsumption(input: { | ||
| db: D1Database | ||
| user: UserWithUsageId | ||
| now: Date | ||
| }): Promise<Array<AdminUsageEntitlementConsumption>> { | ||
| return await Promise.all( | ||
| adminUsageEntitlementResources.map(async (resource) => { | ||
| const current = await readEntitlementResourceUsage({ | ||
| db: input.db, | ||
| userId: input.user.usageUserId, | ||
| resource, | ||
| now: input.now, | ||
| }) | ||
| const limit = input.user.plan | ||
| ? resolvePlanLimit(input.user.plan, resource) | ||
| : null | ||
| const percentOfLimit = | ||
| limit == null || limit === 0 ? null : current / limit | ||
| return { | ||
| resource, | ||
| label: entitlementResourceLabels[resource], | ||
| current, | ||
| limit, | ||
| percentOfLimit, | ||
| overEightyPercent: | ||
| percentOfLimit !== null && percentOfLimit > warningThreshold, | ||
| } | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
N+1 fan-out per user for daily counters, resource counts, and entitlement consumption.
readDailyCounters, readResourceCounts, and readEntitlementConsumption each issue one D1 query per resource per user (1 + 5 + 8 = up to 14 queries for the selected user, 6 for every other page row). With maxPageSize = 100, a single page load can trigger ~600+ D1 queries. Cloudflare D1 only allows up to six simultaneous connections per Worker invocation and each D1 database processes queries serially, so this fan-out will queue heavily and add significant latency (or risk "overloaded" errors under load) rather than scale as intended.
Consider batching these per-resource lookups (e.g., a single query per resource across all usageUserIds, similar to how loadCurrentMonthRollups already batches usage rollups with an IN (...) clause) instead of one query per user per resource.
🤖 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/app/admin-usage-data.ts` around lines 256 - 323, The
per-user resource readers are causing an N+1 query fan-out in the admin usage
page. Update `readDailyCounters`, `readResourceCounts`, and
`readEntitlementConsumption` to batch `readEntitlementResourceUsage` lookups
across users/resources instead of querying once per user per resource, using the
same pattern as `loadCurrentMonthRollups` with an `IN (...)` style batch query.
Keep the existing return shapes in `AdminUsageDailyCounter`,
`AdminUsageResourceCount`, and `AdminUsageEntitlementConsumption`, but populate
them from the batched results rather than individual calls.
Comment on lines
+294
to
+323
| async function readEntitlementConsumption(input: { | ||
| db: D1Database | ||
| user: UserWithUsageId | ||
| now: Date | ||
| }): Promise<Array<AdminUsageEntitlementConsumption>> { | ||
| return await Promise.all( | ||
| adminUsageEntitlementResources.map(async (resource) => { | ||
| const current = await readEntitlementResourceUsage({ | ||
| db: input.db, | ||
| userId: input.user.usageUserId, | ||
| resource, | ||
| now: input.now, | ||
| }) | ||
| const limit = input.user.plan | ||
| ? resolvePlanLimit(input.user.plan, resource) | ||
| : null | ||
| const percentOfLimit = | ||
| limit == null || limit === 0 ? null : current / limit | ||
| return { | ||
| resource, | ||
| label: entitlementResourceLabels[resource], | ||
| current, | ||
| limit, | ||
| percentOfLimit, | ||
| overEightyPercent: | ||
| percentOfLimit !== null && percentOfLimit > warningThreshold, | ||
| } | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
percentOfLimit masks violations when limit === 0.
resolvePlanLimit returns 0 for persistent_package_services when the plan disallows it entirely (per packageServicePersistentAllowed ? null : 0). But Line 311 treats limit === 0 the same as limit == null (unlimited), setting percentOfLimit to null and thus overEightyPercent to false — so this admin dashboard can never surface a warning for a user who has non-zero usage of a resource their plan explicitly forbids, which is exactly the kind of anomaly this dashboard exists to catch.
🐛 Suggested fix
- const percentOfLimit =
- limit == null || limit === 0 ? null : current / limit
+ const percentOfLimit =
+ limit == null ? null : limit === 0 ? (current > 0 ? 1 : 0) : current / limit📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function readEntitlementConsumption(input: { | |
| db: D1Database | |
| user: UserWithUsageId | |
| now: Date | |
| }): Promise<Array<AdminUsageEntitlementConsumption>> { | |
| return await Promise.all( | |
| adminUsageEntitlementResources.map(async (resource) => { | |
| const current = await readEntitlementResourceUsage({ | |
| db: input.db, | |
| userId: input.user.usageUserId, | |
| resource, | |
| now: input.now, | |
| }) | |
| const limit = input.user.plan | |
| ? resolvePlanLimit(input.user.plan, resource) | |
| : null | |
| const percentOfLimit = | |
| limit == null || limit === 0 ? null : current / limit | |
| return { | |
| resource, | |
| label: entitlementResourceLabels[resource], | |
| current, | |
| limit, | |
| percentOfLimit, | |
| overEightyPercent: | |
| percentOfLimit !== null && percentOfLimit > warningThreshold, | |
| } | |
| }), | |
| ) | |
| } | |
| async function readEntitlementConsumption(input: { | |
| db: D1Database | |
| user: UserWithUsageId | |
| now: Date | |
| }): Promise<Array<AdminUsageEntitlementConsumption>> { | |
| return await Promise.all( | |
| adminUsageEntitlementResources.map(async (resource) => { | |
| const current = await readEntitlementResourceUsage({ | |
| db: input.db, | |
| userId: input.user.usageUserId, | |
| resource, | |
| now: input.now, | |
| }) | |
| const limit = input.user.plan | |
| ? resolvePlanLimit(input.user.plan, resource) | |
| : null | |
| const percentOfLimit = | |
| limit == null ? null : limit === 0 ? (current > 0 ? 1 : 0) : current / limit | |
| return { | |
| resource, | |
| label: entitlementResourceLabels[resource], | |
| current, | |
| limit, | |
| percentOfLimit, | |
| overEightyPercent: | |
| percentOfLimit !== null && percentOfLimit > warningThreshold, | |
| } | |
| }), | |
| ) | |
| } |
🤖 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/app/admin-usage-data.ts` around lines 294 - 323,
`readEntitlementConsumption` is treating a zero limit as if the resource were
unlimited, which hides plan violations. Update the
`percentOfLimit`/`overEightyPercent` logic in `readEntitlementConsumption` so
that `limit === 0` is handled as a hard cap (not null), and only `null` means
unlimited; keep using `resolvePlanLimit` and the returned `current` value to
surface warnings when usage is non-zero against a zero limit.
| typeof propertySchema === 'object' && | ||
| !Array.isArray(propertySchema) && | ||
| 'default' in propertySchema | ||
| 'home' in propertySchema |
Contributor
There was a problem hiding this comment.
Wrong property key in schema
Medium Severity
normalizeJsonSchemaDefaultsOptional treats a required field as optional only when the property schema contains a key named home, not default. Zod-derived capability JSON schemas no longer drop defaulted fields from required, so inputs can be rejected or documented as mandatory when they should be optional.
Reviewed by Cursor Bugbot for commit ad1ceca. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/worker/src/mcp/executor.ts (1)
44-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
defaultinjavascriptReservedWords.homeis not a JS reserved word, so the current list can let invalid identifiers through and reject a valid name.🤖 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/executor.ts` around lines 44 - 94, The javascriptReservedWords set in executor.ts is missing the actual JavaScript reserved word default and incorrectly includes home, which is not reserved, so update the reserved-words list used by the executor logic to restore default and remove the invalid entry. Make the change in the javascriptReservedWords declaration so any identifier validation that depends on it correctly rejects default and no longer blocks home.packages/worker/src/package-runtime/package-app.ts (1)
120-128: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError message mixes "kody" and "capabilities" terminology.
The thrown error refers to a "flat kody function" but the actual namespace/property being used is
capabilities(capabilities.remote[connectorName]...). This is confusing for package authors reading the runtime error.✏️ Proposed fix
if (property.startsWith('remote:')) { throw new Error( - \`Remote connector capability "\${property}" is not available as a flat kody function. Use capabilities.remote[connectorName].capabilityName(input) instead.\`, + \`Remote connector capability "\${property}" is not available as a flat capabilities function. Use capabilities.remote[connectorName].capabilityName(input) instead.\`, ); }🤖 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 120 - 128, The thrown error in the Proxy get handler mixes outdated “kody” wording with the actual `capabilities` API, which is confusing for package authors. Update the message in `package-app.ts` inside the `get` trap for `remote:` properties to consistently reference the `capabilities.remote[connectorName].capabilityName(input)` namespace and remove the “flat kody function” terminology, keeping the guidance aligned with the real runtime API.docs/contributing/architecture/remote-connectors.md (1)
9-11: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent placeholder naming for the same URL segment.
The URL pattern at Line 11 still uses
<instanceId>while the updated checklist example at Line 125 uses<connectorName>for the identical WebSocket URL segment. Since exact hello/URL matching matters (a mismatch closes the session per Line 18-20), keep the placeholder name consistent throughout the doc.📝 Proposed fix
-`wss://<worker-origin>/@<username>/connectors/<kind>/<instanceId>` +`wss://<worker-origin>/@<username>/connectors/<kind>/<connectorName>`Also applies to: 124-125
🤖 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 `@docs/contributing/architecture/remote-connectors.md` around lines 9 - 11, The WebSocket URL placeholder name is inconsistent across the remote connector docs, which can confuse readers and imply different session identifiers for the same path segment. Update the URL pattern in the “URLs and session keys” section to use the same placeholder name as the checklist example, and make the naming consistent everywhere this URL is referenced in the document, including the hello/URL matching guidance. Use the existing remote connector URL examples and session-key wording to locate and align the relevant placeholders.
🧹 Nitpick comments (1)
packages/worker/src/package-runtime/package-app.node.test.ts (1)
27-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate string-normalization logic between extractor helpers.
The
.replaceAll('\\\\', '\\').replaceAll('\\\', '`').replaceAll('\${', '${')chain here duplicates the same normalization used by the workflows extractor (extractCreatePackageAppWorkerSource, lines 19-25). Consider factoring this into a shared helper (e.g.unescapeExtractedSource(text: string)`) used by both extractors.♻️ Proposed refactor
+function unescapeExtractedSource(text: string) { + return text + .replaceAll('\\\\', '\\') + .replaceAll('\\`', '`') + .replaceAll('\\${', '${') +} + async function extractCreateCapabilitiesProxySource() { const sourceText = await readFile( new URL('./package-app.ts', import.meta.url), 'utf8', ) const start = sourceText.indexOf( 'function createCapabilitiesProxy(runtimeBridge) {', ) const end = sourceText.indexOf('\nfunction createStorageProxy', start) if (start < 0 || end < 0) { throw new Error('createCapabilitiesProxy source was not found.') } - const proxySource = sourceText - .slice(start, end) - .replaceAll('\\\\', '\\') - .replaceAll('\\`', '`') - .replaceAll('\\${', '${') + const proxySource = unescapeExtractedSource(sourceText.slice(start, end)) return `${proxySource}; return createCapabilitiesProxy(runtimeBridge);` }🤖 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.node.test.ts` around lines 27 - 45, The source-normalization chain in extractCreateCapabilitiesProxySource duplicates the same unescaping logic used by extractCreatePackageAppWorkerSource. Factor the repeated .replaceAll(...) sequence into a shared helper such as unescapeExtractedSource(text), and update both extractor helpers to call it so the normalization behavior lives in one place.
🤖 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/app/account-export.node.test.ts`:
- Around line 221-223: The password-hash assertion in
account-export.node.test.ts is too weak because `expect.not.objectContaining({
password_hash: expect.anything() })` still allows nullish values; update the
`accountExport.d1.users.rows` expectation to mirror the other secret-field
checks and assert that `password_hash` is not present at all. Use the existing
`accountExport` and `rows` matcher block to locate the test and replace the
matcher with one that verifies the property is absent.
In `@packages/worker/src/mcp/capabilities/define-capability.ts`:
- Line 218: The optionality check in normalizeJsonSchemaDefaultsOptional is
using the wrong schema key, so defaulted properties are never removed from
required. Update the propertySchema check in define-capability.ts to look for
the JSON Schema default keyword instead of home, and keep the existing logic
that builds optionalByDefault so Zod fields with defaults become optional.
In `@packages/worker/src/mcp/capabilities/packages/get-git-remote.ts`:
- Around line 104-108: The Git remote refspec configuration in get-git-remote is
written under the wrong remote name, so the fetch settings never apply to the
actual remote created by the command sequence. Update the remote config entries
in the getGitRemote flow to use the same identifier as git remote add and the
later fetch/push calls, namely kody, so the default heads and notes refspecs are
attached to the correct remote.
In `@packages/worker/src/mcp/server-instructions.ts`:
- Line 100: Update the MCP instructions text in server-instructions so the
built-in capabilities sentence matches the examples and does not imply calling
capabilities through kody directly. In the instruction string near the
capability examples, replace the misleading “available through kody” wording
with language that clearly says built-in capabilities are accessed via the
capabilities object exposed by kody:runtime, using the existing capability_id
and ["capability-id"] patterns. Keep the remote connector and workflows wording
unchanged.
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Line 830: The user-facing description in the search capability detail output
is inconsistent with the example generated by buildCapabilityExecuteExample.
Update the narrative text in search-format.ts so it refers to the imported
capabilities object and its methods, matching the example usage instead of
mentioning the kody object. Keep the wording aligned with the execute example
output to avoid confusing readers.
---
Outside diff comments:
In `@docs/contributing/architecture/remote-connectors.md`:
- Around line 9-11: The WebSocket URL placeholder name is inconsistent across
the remote connector docs, which can confuse readers and imply different session
identifiers for the same path segment. Update the URL pattern in the “URLs and
session keys” section to use the same placeholder name as the checklist example,
and make the naming consistent everywhere this URL is referenced in the
document, including the hello/URL matching guidance. Use the existing remote
connector URL examples and session-key wording to locate and align the relevant
placeholders.
In `@packages/worker/src/mcp/executor.ts`:
- Around line 44-94: The javascriptReservedWords set in executor.ts is missing
the actual JavaScript reserved word default and incorrectly includes home, which
is not reserved, so update the reserved-words list used by the executor logic to
restore default and remove the invalid entry. Make the change in the
javascriptReservedWords declaration so any identifier validation that depends on
it correctly rejects default and no longer blocks home.
In `@packages/worker/src/package-runtime/package-app.ts`:
- Around line 120-128: The thrown error in the Proxy get handler mixes outdated
“kody” wording with the actual `capabilities` API, which is confusing for
package authors. Update the message in `package-app.ts` inside the `get` trap
for `remote:` properties to consistently reference the
`capabilities.remote[connectorName].capabilityName(input)` namespace and remove
the “flat kody function” terminology, keeping the guidance aligned with the real
runtime API.
---
Nitpick comments:
In `@packages/worker/src/package-runtime/package-app.node.test.ts`:
- Around line 27-45: The source-normalization chain in
extractCreateCapabilitiesProxySource duplicates the same unescaping logic used
by extractCreatePackageAppWorkerSource. Factor the repeated .replaceAll(...)
sequence into a shared helper such as unescapeExtractedSource(text), and update
both extractor helpers to call it so the normalization behavior lives in one
place.
🪄 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: cc12e1b7-0f24-46a5-9059-5bd98e300fb7
📒 Files selected for processing (97)
docs/contributing/adding-capabilities.mddocs/contributing/architecture/capability-and-primitive-audit-2026-07.mddocs/contributing/architecture/data-storage.mddocs/contributing/architecture/index.mddocs/contributing/architecture/primitives.yamldocs/contributing/architecture/remote-connectors.mddocs/contributing/architecture/usage-metering.mddocs/contributing/packages-and-manifests.mddocs/contributing/security.mddocs/guides/secret-backed-integration.mddocs/use/execute.mddocs/use/packages.mddocs/use/repo-sessions.mddocs/use/search.mddocs/use/secrets-and-values.mdpackages/shared/src/remote-connectors.tspackages/worker/client/mcp-apps/kody-ui-utils.node.test.tspackages/worker/client/mcp-apps/kody-ui-utils.tspackages/worker/client/mcp-apps/kody-widget-runtime.tspackages/worker/client/routes/account-remote-connectors.tsxpackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-export.node.test.tspackages/worker/src/app/email/cloudflare-email.node.test.tspackages/worker/src/app/handlers/account-remote-connectors.node.test.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/package-subscriptions.tspackages/worker/src/index.tspackages/worker/src/jobs/reconcile-artifacts-pushes.node.test.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/capabilities-remote-types.tspackages/worker/src/mcp/capabilities/define-capability.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/meta/domain.tspackages/worker/src/mcp/capabilities/meta/execute.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.node.test.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.tspackages/worker/src/mcp/capabilities/meta/search-and-execute.node.test.tspackages/worker/src/mcp/capabilities/packages/get-git-remote.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.tspackages/worker/src/mcp/capabilities/remote-connector/index.tspackages/worker/src/mcp/capabilities/secrets/secret-list.tspackages/worker/src/mcp/execute-modules/authenticated-fetch.node.test.tspackages/worker/src/mcp/execute-modules/capabilities-runtime-utils.node.test.tspackages/worker/src/mcp/execute-modules/capabilities-runtime-utils.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/src/mcp/run-capabilities-registry.node.test.tspackages/worker/src/mcp/run-capabilities-registry.tspackages/worker/src/mcp/server-instructions.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search-handler.node.test.tspackages/worker/src/mcp/tools/search.node.test.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-invocations/service.tspackages/worker/src/package-retrievers/service.tspackages/worker/src/package-runtime/module-graph.node.test.tspackages/worker/src/package-runtime/module-graph.tspackages/worker/src/package-runtime/module-graph.workers.test.tspackages/worker/src/package-runtime/package-app.node.test.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/package-workflows.node.test.tspackages/worker/src/package-runtime/package-workflows.tspackages/worker/src/package-runtime/runtime-isolation.node.test.tspackages/worker/src/remote-connector/connector-session-key.node.test.tspackages/worker/src/remote-connector/connector-session-key.tspackages/worker/src/remote-connector/remote-domain-id.node.test.tspackages/worker/src/remote-connector/remote-domain-id.tspackages/worker/src/remote-connector/resolve-remote-connector-secret.node.test.tspackages/worker/src/remote-connector/session.node.test.tspackages/worker/src/remote-connector/settings-service.node.test.tspackages/worker/src/remote-connector/settings-service.tspackages/worker/src/remote-connector/snapshot-cache.node.test.tspackages/worker/src/repo/checks.node.test.tspackages/worker/src/repo/checks.tspackages/worker/src/repo/external-publish.node.test.tspackages/worker/src/repo/manifest.tspackages/worker/src/repo/repo-capabilities-execution.tspackages/worker/src/repo/repo-session-do.node.test.tspackages/worker/src/repo/source-service.tspackages/worker/src/repo/source-sync.node.test.tspackages/worker/src/repo/source-templates.tspackages/worker/src/security/public-route-hardening.workers.test.tspackages/worker/src/storage-runner.tspackages/worker/src/test-support/cloudflare-workers-stub.ts
✅ Files skipped from review due to trivial changes (19)
- packages/worker/src/mcp/capabilities/secrets/secret-list.ts
- packages/worker/src/mcp/capabilities/meta/domain.ts
- docs/use/repo-sessions.md
- docs/contributing/architecture/index.md
- packages/worker/src/mcp-auth.workers.test.ts
- packages/worker/src/repo/source-templates.ts
- packages/worker/src/mcp/capabilities/meta/search-and-execute.node.test.ts
- packages/worker/src/mcp/fetch-gateway.node.test.ts
- packages/worker/src/mcp/capabilities/domain-metadata.ts
- docs/contributing/security.md
- docs/contributing/packages-and-manifests.md
- packages/worker/src/email/package-subscriptions.ts
- packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts
- packages/worker/src/app/email/cloudflare-email.node.test.ts
- packages/worker/src/repo/external-publish.node.test.ts
- docs/use/secrets-and-values.md
- docs/contributing/architecture/capability-and-primitive-audit-2026-07.md
- docs/contributing/adding-capabilities.md
- packages/worker/src/remote-connector/snapshot-cache.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/worker/src/remote-connector/remote-domain-id.node.test.ts
- docs/use/search.md
- packages/worker/src/mcp/tools/execute.node.test.ts
- packages/worker/src/mcp/capabilities/meta/meta-list-capabilities.ts
- packages/worker/src/mcp/tools/search.ts
- packages/worker/src/mcp/tools/execute.ts
- packages/worker/src/mcp/capabilities/remote-connector/index.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/worker/src/mcp/executor.ts (1)
44-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
defaultinjavascriptReservedWords.homeis not a JS reserved word, so the current list can let invalid identifiers through and reject a valid name.🤖 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/executor.ts` around lines 44 - 94, The javascriptReservedWords set in executor.ts is missing the actual JavaScript reserved word default and incorrectly includes home, which is not reserved, so update the reserved-words list used by the executor logic to restore default and remove the invalid entry. Make the change in the javascriptReservedWords declaration so any identifier validation that depends on it correctly rejects default and no longer blocks home.packages/worker/src/package-runtime/package-app.ts (1)
120-128: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError message mixes "kody" and "capabilities" terminology.
The thrown error refers to a "flat kody function" but the actual namespace/property being used is
capabilities(capabilities.remote[connectorName]...). This is confusing for package authors reading the runtime error.✏️ Proposed fix
if (property.startsWith('remote:')) { throw new Error( - \`Remote connector capability "\${property}" is not available as a flat kody function. Use capabilities.remote[connectorName].capabilityName(input) instead.\`, + \`Remote connector capability "\${property}" is not available as a flat capabilities function. Use capabilities.remote[connectorName].capabilityName(input) instead.\`, ); }🤖 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 120 - 128, The thrown error in the Proxy get handler mixes outdated “kody” wording with the actual `capabilities` API, which is confusing for package authors. Update the message in `package-app.ts` inside the `get` trap for `remote:` properties to consistently reference the `capabilities.remote[connectorName].capabilityName(input)` namespace and remove the “flat kody function” terminology, keeping the guidance aligned with the real runtime API.docs/contributing/architecture/remote-connectors.md (1)
9-11: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent placeholder naming for the same URL segment.
The URL pattern at Line 11 still uses
<instanceId>while the updated checklist example at Line 125 uses<connectorName>for the identical WebSocket URL segment. Since exact hello/URL matching matters (a mismatch closes the session per Line 18-20), keep the placeholder name consistent throughout the doc.📝 Proposed fix
-`wss://<worker-origin>/@<username>/connectors/<kind>/<instanceId>` +`wss://<worker-origin>/@<username>/connectors/<kind>/<connectorName>`Also applies to: 124-125
🤖 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 `@docs/contributing/architecture/remote-connectors.md` around lines 9 - 11, The WebSocket URL placeholder name is inconsistent across the remote connector docs, which can confuse readers and imply different session identifiers for the same path segment. Update the URL pattern in the “URLs and session keys” section to use the same placeholder name as the checklist example, and make the naming consistent everywhere this URL is referenced in the document, including the hello/URL matching guidance. Use the existing remote connector URL examples and session-key wording to locate and align the relevant placeholders.
🧹 Nitpick comments (1)
packages/worker/src/package-runtime/package-app.node.test.ts (1)
27-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate string-normalization logic between extractor helpers.
The
.replaceAll('\\\\', '\\').replaceAll('\\\', '`').replaceAll('\${', '${')chain here duplicates the same normalization used by the workflows extractor (extractCreatePackageAppWorkerSource, lines 19-25). Consider factoring this into a shared helper (e.g.unescapeExtractedSource(text: string)`) used by both extractors.♻️ Proposed refactor
+function unescapeExtractedSource(text: string) { + return text + .replaceAll('\\\\', '\\') + .replaceAll('\\`', '`') + .replaceAll('\\${', '${') +} + async function extractCreateCapabilitiesProxySource() { const sourceText = await readFile( new URL('./package-app.ts', import.meta.url), 'utf8', ) const start = sourceText.indexOf( 'function createCapabilitiesProxy(runtimeBridge) {', ) const end = sourceText.indexOf('\nfunction createStorageProxy', start) if (start < 0 || end < 0) { throw new Error('createCapabilitiesProxy source was not found.') } - const proxySource = sourceText - .slice(start, end) - .replaceAll('\\\\', '\\') - .replaceAll('\\`', '`') - .replaceAll('\\${', '${') + const proxySource = unescapeExtractedSource(sourceText.slice(start, end)) return `${proxySource}; return createCapabilitiesProxy(runtimeBridge);` }🤖 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.node.test.ts` around lines 27 - 45, The source-normalization chain in extractCreateCapabilitiesProxySource duplicates the same unescaping logic used by extractCreatePackageAppWorkerSource. Factor the repeated .replaceAll(...) sequence into a shared helper such as unescapeExtractedSource(text), and update both extractor helpers to call it so the normalization behavior lives in one place.
🤖 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/app/account-export.node.test.ts`:
- Around line 221-223: The password-hash assertion in
account-export.node.test.ts is too weak because `expect.not.objectContaining({
password_hash: expect.anything() })` still allows nullish values; update the
`accountExport.d1.users.rows` expectation to mirror the other secret-field
checks and assert that `password_hash` is not present at all. Use the existing
`accountExport` and `rows` matcher block to locate the test and replace the
matcher with one that verifies the property is absent.
In `@packages/worker/src/mcp/capabilities/define-capability.ts`:
- Line 218: The optionality check in normalizeJsonSchemaDefaultsOptional is
using the wrong schema key, so defaulted properties are never removed from
required. Update the propertySchema check in define-capability.ts to look for
the JSON Schema default keyword instead of home, and keep the existing logic
that builds optionalByDefault so Zod fields with defaults become optional.
In `@packages/worker/src/mcp/capabilities/packages/get-git-remote.ts`:
- Around line 104-108: The Git remote refspec configuration in get-git-remote is
written under the wrong remote name, so the fetch settings never apply to the
actual remote created by the command sequence. Update the remote config entries
in the getGitRemote flow to use the same identifier as git remote add and the
later fetch/push calls, namely kody, so the default heads and notes refspecs are
attached to the correct remote.
In `@packages/worker/src/mcp/server-instructions.ts`:
- Line 100: Update the MCP instructions text in server-instructions so the
built-in capabilities sentence matches the examples and does not imply calling
capabilities through kody directly. In the instruction string near the
capability examples, replace the misleading “available through kody” wording
with language that clearly says built-in capabilities are accessed via the
capabilities object exposed by kody:runtime, using the existing capability_id
and ["capability-id"] patterns. Keep the remote connector and workflows wording
unchanged.
In `@packages/worker/src/mcp/tools/search-format.ts`:
- Line 830: The user-facing description in the search capability detail output
is inconsistent with the example generated by buildCapabilityExecuteExample.
Update the narrative text in search-format.ts so it refers to the imported
capabilities object and its methods, matching the example usage instead of
mentioning the kody object. Keep the wording aligned with the execute example
output to avoid confusing readers.
---
Outside diff comments:
In `@docs/contributing/architecture/remote-connectors.md`:
- Around line 9-11: The WebSocket URL placeholder name is inconsistent across
the remote connector docs, which can confuse readers and imply different session
identifiers for the same path segment. Update the URL pattern in the “URLs and
session keys” section to use the same placeholder name as the checklist example,
and make the naming consistent everywhere this URL is referenced in the
document, including the hello/URL matching guidance. Use the existing remote
connector URL examples and session-key wording to locate and align the relevant
placeholders.
In `@packages/worker/src/mcp/executor.ts`:
- Around line 44-94: The javascriptReservedWords set in executor.ts is missing
the actual JavaScript reserved word default and incorrectly includes home, which
is not reserved, so update the reserved-words list used by the executor logic to
restore default and remove the invalid entry. Make the change in the
javascriptReservedWords declaration so any identifier validation that depends on
it correctly rejects default and no longer blocks home.
In `@packages/worker/src/package-runtime/package-app.ts`:
- Around line 120-128: The thrown error in the Proxy get handler mixes outdated
“kody” wording with the actual `capabilities` API, which is confusing for
package authors. Update the message in `package-app.ts` inside the `get` trap
for `remote:` properties to consistently reference the
`capabilities.remote[connectorName].capabilityName(input)` namespace and remove
the “flat kody function” terminology, keeping the guidance aligned with the real
runtime API.
---
Nitpick comments:
In `@packages/worker/src/package-runtime/package-app.node.test.ts`:
- Around line 27-45: The source-normalization chain in
extractCreateCapabilitiesProxySource duplicates the same unescaping logic used
by extractCreatePackageAppWorkerSource. Factor the repeated .replaceAll(...)
sequence into a shared helper such as unescapeExtractedSource(text), and update
both extractor helpers to call it so the normalization behavior lives in one
place.
🪄 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: cc12e1b7-0f24-46a5-9059-5bd98e300fb7
📒 Files selected for processing (97)
docs/contributing/adding-capabilities.mddocs/contributing/architecture/capability-and-primitive-audit-2026-07.mddocs/contributing/architecture/data-storage.mddocs/contributing/architecture/index.mddocs/contributing/architecture/primitives.yamldocs/contributing/architecture/remote-connectors.mddocs/contributing/architecture/usage-metering.mddocs/contributing/packages-and-manifests.mddocs/contributing/security.mddocs/guides/secret-backed-integration.mddocs/use/execute.mddocs/use/packages.mddocs/use/repo-sessions.mddocs/use/search.mddocs/use/secrets-and-values.mdpackages/shared/src/remote-connectors.tspackages/worker/client/mcp-apps/kody-ui-utils.node.test.tspackages/worker/client/mcp-apps/kody-ui-utils.tspackages/worker/client/mcp-apps/kody-widget-runtime.tspackages/worker/client/routes/account-remote-connectors.tsxpackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-export.node.test.tspackages/worker/src/app/email/cloudflare-email.node.test.tspackages/worker/src/app/handlers/account-remote-connectors.node.test.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/package-subscriptions.tspackages/worker/src/index.tspackages/worker/src/jobs/reconcile-artifacts-pushes.node.test.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/capabilities-remote-types.tspackages/worker/src/mcp/capabilities/define-capability.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/meta/domain.tspackages/worker/src/mcp/capabilities/meta/execute.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.node.test.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.tspackages/worker/src/mcp/capabilities/meta/search-and-execute.node.test.tspackages/worker/src/mcp/capabilities/packages/get-git-remote.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.tspackages/worker/src/mcp/capabilities/remote-connector/index.tspackages/worker/src/mcp/capabilities/secrets/secret-list.tspackages/worker/src/mcp/execute-modules/authenticated-fetch.node.test.tspackages/worker/src/mcp/execute-modules/capabilities-runtime-utils.node.test.tspackages/worker/src/mcp/execute-modules/capabilities-runtime-utils.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/src/mcp/run-capabilities-registry.node.test.tspackages/worker/src/mcp/run-capabilities-registry.tspackages/worker/src/mcp/server-instructions.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search-handler.node.test.tspackages/worker/src/mcp/tools/search.node.test.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-invocations/service.tspackages/worker/src/package-retrievers/service.tspackages/worker/src/package-runtime/module-graph.node.test.tspackages/worker/src/package-runtime/module-graph.tspackages/worker/src/package-runtime/module-graph.workers.test.tspackages/worker/src/package-runtime/package-app.node.test.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/package-workflows.node.test.tspackages/worker/src/package-runtime/package-workflows.tspackages/worker/src/package-runtime/runtime-isolation.node.test.tspackages/worker/src/remote-connector/connector-session-key.node.test.tspackages/worker/src/remote-connector/connector-session-key.tspackages/worker/src/remote-connector/remote-domain-id.node.test.tspackages/worker/src/remote-connector/remote-domain-id.tspackages/worker/src/remote-connector/resolve-remote-connector-secret.node.test.tspackages/worker/src/remote-connector/session.node.test.tspackages/worker/src/remote-connector/settings-service.node.test.tspackages/worker/src/remote-connector/settings-service.tspackages/worker/src/remote-connector/snapshot-cache.node.test.tspackages/worker/src/repo/checks.node.test.tspackages/worker/src/repo/checks.tspackages/worker/src/repo/external-publish.node.test.tspackages/worker/src/repo/manifest.tspackages/worker/src/repo/repo-capabilities-execution.tspackages/worker/src/repo/repo-session-do.node.test.tspackages/worker/src/repo/source-service.tspackages/worker/src/repo/source-sync.node.test.tspackages/worker/src/repo/source-templates.tspackages/worker/src/security/public-route-hardening.workers.test.tspackages/worker/src/storage-runner.tspackages/worker/src/test-support/cloudflare-workers-stub.ts
✅ Files skipped from review due to trivial changes (19)
- packages/worker/src/mcp/capabilities/secrets/secret-list.ts
- packages/worker/src/mcp/capabilities/meta/domain.ts
- docs/use/repo-sessions.md
- docs/contributing/architecture/index.md
- packages/worker/src/mcp-auth.workers.test.ts
- packages/worker/src/repo/source-templates.ts
- packages/worker/src/mcp/capabilities/meta/search-and-execute.node.test.ts
- packages/worker/src/mcp/fetch-gateway.node.test.ts
- packages/worker/src/mcp/capabilities/domain-metadata.ts
- docs/contributing/security.md
- docs/contributing/packages-and-manifests.md
- packages/worker/src/email/package-subscriptions.ts
- packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts
- packages/worker/src/app/email/cloudflare-email.node.test.ts
- packages/worker/src/repo/external-publish.node.test.ts
- docs/use/secrets-and-values.md
- docs/contributing/architecture/capability-and-primitive-audit-2026-07.md
- docs/contributing/adding-capabilities.md
- packages/worker/src/remote-connector/snapshot-cache.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/worker/src/remote-connector/remote-domain-id.node.test.ts
- docs/use/search.md
- packages/worker/src/mcp/tools/execute.node.test.ts
- packages/worker/src/mcp/capabilities/meta/meta-list-capabilities.ts
- packages/worker/src/mcp/tools/search.ts
- packages/worker/src/mcp/tools/execute.ts
- packages/worker/src/mcp/capabilities/remote-connector/index.ts
🛑 Comments failed to post (5)
packages/worker/src/app/account-export.node.test.ts (1)
221-223: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Tighten the password-hash redaction check.
This matcher still passes if
password_hashis present but nullish, so it doesn't fully enforce the secrecy contract. Mirror the other secret-field assertions and check that the property is absent.💡 Proposed fix
- expect(accountExport.d1.users.rows).toEqual([ - expect.not.objectContaining({ password_hash: expect.anything() }), - ]) + expect(accountExport.d1.users.rows).toHaveLength(1) + expect(accountExport.d1.users.rows[0]).not.toHaveProperty('password_hash')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.expect(accountExport.d1.users.rows).toHaveLength(1) expect(accountExport.d1.users.rows[0]).not.toHaveProperty('password_hash')🤖 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/app/account-export.node.test.ts` around lines 221 - 223, The password-hash assertion in account-export.node.test.ts is too weak because `expect.not.objectContaining({ password_hash: expect.anything() })` still allows nullish values; update the `accountExport.d1.users.rows` expectation to mirror the other secret-field checks and assert that `password_hash` is not present at all. Use the existing `accountExport` and `rows` matcher block to locate the test and replace the matcher with one that verifies the property is absent.packages/worker/src/mcp/capabilities/define-capability.ts (1)
218-218: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Bug:
'home'check breaks default-based optionality.
normalizeJsonSchemaDefaultsOptionalexists to drop properties carrying a JSON Schemadefaultfrom therequiredlist (so Zod.default()fields become optional). JSON Schema has nohomekeyword, so'home' in propertySchemanever matches —optionalByDefaultis always empty and defaulted fields stayrequired. This looks like collateral from the'default'→'home'package-action rename insearch-format.ts.🐛 Proposed fix
- 'home' in propertySchema + 'default' in propertySchema📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'default' in propertySchema🤖 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/capabilities/define-capability.ts` at line 218, The optionality check in normalizeJsonSchemaDefaultsOptional is using the wrong schema key, so defaulted properties are never removed from required. Update the propertySchema check in define-capability.ts to look for the JSON Schema default keyword instead of home, and keep the existing logic that builds optionalByDefault so Zod fields with defaults become optional.packages/worker/src/mcp/capabilities/packages/get-git-remote.ts (1)
104-108: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Git config targets wrong remote name —
capabilitiesvs.kody.The remote is created as
kody(line 104), and fetch/push later targetkody(lines 107-108), but the fetch refspecs are configured underremote.capabilities.fetch(lines 105-106). Since no remote namedcapabilitiesexists, these config entries are inert and never apply to the actualkodyremote — silently breaking the intended default heads+notes fetch behavior for any subsequentgit fetch kodythe agent runs without an explicit refspec.This looks like an over-broad
kody→capabilitiesrename applied to a literal Git remote identifier that must match the name passed togit remote add.🐛 Proposed fix to target the correct remote name
- `git config remote.capabilities.fetch '+refs/heads/*:refs/remotes/kody/*'`, - `git config --add remote.capabilities.fetch '+refs/notes/*:refs/notes/*'`, + `git config remote.kody.fetch '+refs/heads/*:refs/remotes/kody/*'`, + `git config --add remote.kody.fetch '+refs/notes/*:refs/notes/*'`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`git remote add kody ${shellQuote(sourceHead.remote)}`, `git config remote.kody.fetch '+refs/heads/*:refs/remotes/kody/*'`, `git config --add remote.kody.fetch '+refs/notes/*:refs/notes/*'`, `git -c http.extraHeader=${shellQuote(gitExtraHeader)} fetch kody 'refs/notes/*:refs/notes/*'`, `git -c http.extraHeader=${shellQuote(gitExtraHeader)} push kody HEAD:${shellQuote(sourceHead.defaultBranch)}`,🤖 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/capabilities/packages/get-git-remote.ts` around lines 104 - 108, The Git remote refspec configuration in get-git-remote is written under the wrong remote name, so the fetch settings never apply to the actual remote created by the command sequence. Update the remote config entries in the getGitRemote flow to use the same identifier as git remote add and the later fetch/push calls, namely kody, so the default heads and notes refspecs are attached to the correct remote.packages/worker/src/mcp/server-instructions.ts (1)
100-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix "available through
kody" wording — contradicts the example calls.The sentence says built-in capabilities are "available through
kody" but the very next examples callcapabilities.capability_id(input)/capabilities["capability-id"](input), notkody.xxx(...). Since this string is served verbatim as MCP server instructions, this wording could lead an agent to emit invalid code (kody.capability_id(...)).📝 Proposed fix
-Built-in capabilities returned by \`search\` are available through \`kody\`: use \`await capabilities.capability_id(input)\` for valid identifier names or \`await capabilities["capability-id"](input)\` for non-identifier ids. +Built-in capabilities returned by \`search\` are available through \`capabilities\`: use \`await capabilities.capability_id(input)\` for valid identifier names or \`await capabilities["capability-id"](input)\` for non-identifier ids.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- Single ESM module string with a default export such as `export default async function main(input = {}) { ... }`; `params` are passed as the first argument. Import runtime APIs from `kody:runtime`. Example: `import { capabilities, refreshAccessToken, createAuthenticatedFetch, oauthClientCredentials, secretHeaders, workflows } from 'kody:runtime'`. Built-in capabilities returned by `search` are available through `capabilities`: use `await capabilities.capability_id(input)` for valid identifier names or `await capabilities["capability-id"](input)` for non-identifier ids. Remote connector capabilities are deliberately separate: use `await capabilities.remote["name"].capability_name(input)` (for example `capabilities.remote["home"].set_pin({ pin })`), never a flat `capabilities.kind_instance_capability(... )` call. `workflows.create` can queue either inline `code` or a saved-package `exportName` through the shared Workflow hub. Use `secretHeaders.basic(...)` or `oauthClientCredentials(...)` for client-credentials Basic Auth instead of asking users to precompute a Basic header. Prefer one `execute` when the plan is clear. Full rules for `fetch`, placeholders, `secret_list` / `value_get`, and `x-kody-secret`: see the `execute` tool description.🤖 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/server-instructions.ts` at line 100, Update the MCP instructions text in server-instructions so the built-in capabilities sentence matches the examples and does not imply calling capabilities through kody directly. In the instruction string near the capability examples, replace the misleading “available through kody” wording with language that clearly says built-in capabilities are accessed via the capabilities object exposed by kody:runtime, using the existing capability_id and ["capability-id"] patterns. Keep the remote connector and workflows wording unchanged.packages/worker/src/mcp/tools/search-format.ts (1)
830-830: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
User-facing text mismatches the example object name.
This narrative says methods are on the imported
kodyobject, but the emitted example (buildCapabilityExecuteExample) imports{ capabilities }and callscapabilities.*. Align the wording with the example to avoid confusing readers of the capability detail output.✏️ Proposed fix
- 'Built-in capabilities returned by `search` are available inside `execute` as methods on the imported `kody` object.', + 'Built-in capabilities returned by `search` are available inside `execute` as methods on the imported `capabilities` object.',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.'Built-in capabilities returned by `search` are available inside `execute` as methods on the imported `capabilities` object.',🤖 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/tools/search-format.ts` at line 830, The user-facing description in the search capability detail output is inconsistent with the example generated by buildCapabilityExecuteExample. Update the narrative text in search-format.ts so it refers to the imported capabilities object and its methods, matching the example usage instead of mentioning the kody object. Keep the wording aligned with the execute example output to avoid confusing readers.
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 364ea7c. Configure here.
| export function normalizeRemoteConnectorInstanceId(instanceId: string): string { | ||
| return instanceId.trim() | ||
| return instanceId.trim().toLowerCase() | ||
| } |
Contributor
There was a problem hiding this comment.
Connector name case normalization drift
Medium Severity
normalizeRemoteConnectorInstanceId now lowercases names for session keys and D1 lookups, but existing remote_connector_settings.instance_id values are not migrated and global uniqueness compares normalized input to stored strings with case-sensitive equality. Legacy mixed-case rows can fail secret lookup while a second lowercase row slips past the duplicate-name check.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 364ea7c. Configure here.
This was referenced Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Walkthrough
docs/contributing/architecture/capability-and-primitive-audit-2026-07.mdwith migration-risk findings, blast radius, recommendations, safe fixes, and follow-up items.kody:import { kody } from 'kody:runtime'; the oldcodemodeexport and the supersededcapabilitiesexport are intentionally gone.kody.value_get(...)), remote capabilities usekody.remote["connector-name"].tool(input)and are no longer flatkind_instance_toolfunctions.lowercase letters/numbers/dashes), globally unique per user, and keykody.remote[name].remote:<name>:<tool>, with deterministic hash suffixes when names require sanitization and would otherwise collide.source, connector name, clean tool name) in search/detail/meta-list/logging.kody.remoteProxy errors for unknown connector, unknown capability, and disconnected/offline connectors in execute and package apps.secret_jwt_signinput fields to snake_case, and folded Add per-user account data export #629 account export surfaces into the audit/primitive inventory.Migration punch list for Kent
Runtime import/object rename:
import { codemode } from 'kody:runtime'and replace withimport { kody } from 'kody:runtime'.codemode.andcodemode[call sites and replace withkody./kody[.import { capabilities } from 'kody:runtime', replace that withimport { kody } from 'kody:runtime'too.codemodeandcapabilitiesruntime objects are not exported; no aliases/shims exist.Remote connector rename and call sites:
home/defaultconnector setting to explicit connector namehomein/account/remote-connectors./@{username}/connectors/{kind}/{connectorName}with connectorId/namehome.userScopedConnectorSessionKey(userId, kind, instanceId), creating a new RemoteConnectorSession DO id. The old live session snapshot can be orphaned; reconnect rebuilds it from settings.<kind>_<instance>_<tool>->remote:<name>:<tool>.codemode.<kind>_<instance>_<tool>(input)->kody.remote["<name>"].<tool>(input).home_default_bond_shade_set_position->remote:home:bond_shade_set_position; call askody.remote["home"].bond_shade_set_position(input).lighting_default_lutron_set_credentials-> rename connector tolighting; id becomesremote:lighting:lutron_set_credentials; call askody.remote["lighting"].lutron_set_credentials(input).roku_default_roku_press_key-> rename connector toroku; id becomesremote:roku:roku_press_key; call askody.remote["roku"].roku_press_key(input).secret_entries.allowed_capabilitiesrows that referenced old flat remote names.Builtin capability renames:
kody_official_guide->coding_guide_getpackage_subscription_list->package_subscriptions_listworkflow_list->workflow_run_listjwt_sign->secret_jwt_signInput field renames:
secret_jwt_sign.privateKeySecretName->secret_jwt_sign.private_key_secret_namesecret_jwt_sign.privateKeySecretScope->secret_jwt_sign.private_key_secret_scopesecret_jwt_sign.privateKeyJsonField->secret_jwt_sign.private_key_json_fieldOutput field renames:
Account export (#629) notes:
account_export_manifest,account_export_section.Validation
npx vitest run packages/worker/src/package-runtime/runtime-isolation.node.test.ts packages/worker/src/package-runtime/module-graph.node.test.ts packages/worker/src/package-runtime/module-graph.workers.test.ts packages/worker/src/mcp/run-kody-registry.node.test.ts packages/worker/src/mcp/executor.node.test.ts packages/worker/src/mcp/tools/search-format.node.test.ts packages/worker/src/package-runtime/package-app.node.test.ts packages/worker/src/remote-connector/remote-domain-id.node.test.ts packages/worker/src/remote-connector/settings-service.node.test.ts packages/worker/src/remote-connector/connector-session-key.node.test.ts packages/worker/src/remote-connector/session.node.test.tsnpx vitest run packages/worker/src/app/account-export.node.test.ts packages/worker/src/app/account-deletion.node.test.ts packages/worker/src/app/handlers/account-remote-connectors.node.test.ts packages/worker/src/mcp-auth.workers.test.ts packages/worker/src/security/public-route-hardening.workers.test.tsnpm run typechecknpm run format:checknpm run validatenpm run test:pushSystem recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@ current · Head:364ea7cClassification: extends — the PR changes capability registry/search/kody-runtime/package-runtime contracts and architecture docs; it adds no migration.
Primitives touched
capability-registrymcp-serverremote-connectorskody.remote[name].toolProxy,remote:<name>:<tool>idscapabilities-executekodyruntime export only; nocodemodeorcapabilitiesruntime exportpackage-runtimekody.remote[...]account-exportd1-app-dbbundle-artifacts-kvvectorize-searchentitlementsrbacChange flow
Before / after
Invariants
Summary by CodeRabbit
source, plus remote-connector details) across capability discovery, execution examples, and observability.capabilities.remote[...]access patterns and improved connector naming/validation.capabilitiesAPIs and new capability names (e.g.,coding_guide_get,workflow_run_list,package_subscriptions_list,secret_jwt_sign).