Repository navigation
Meter statically imported package export calls (package_static_call) - #1047
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis change adds ChangesStatic package call metering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BundledModule
participant StaticCallMeter
participant RuntimeBridge
participant PackageStaticCallMeterTools
participant UsageSink
BundledModule->>StaticCallMeter: report packageId, durationMs, outcome
BundledModule->>StaticCallMeter: flush buffered events
StaticCallMeter->>RuntimeBridge: record event batch
RuntimeBridge->>PackageStaticCallMeterTools: forward event payload
PackageStaticCallMeterTools->>UsageSink: validate and record package_static_call
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
🔎 Preview deployed: https://kody-pr-1047.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/worker/src/package-runtime/module-graph.node.test.ts (1)
821-824: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the named-export assertion.
toContain('as add')passes on almost any re-export shape. Asserting the emittedexport { … as add }line (or matching the generated local name) makes the test actually pin the codegen contract.🤖 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/module-graph.node.test.ts` around lines 821 - 824, Update the named-export assertion in the module-graph test to match the complete emitted export statement, including the generated local export expression and its alias “add,” rather than asserting only the generic substring “as add.” Keep the existing proxy generation assertion unchanged.packages/worker/src/package-runtime/static-call-metering.workers.test.ts (1)
351-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe forged-stamp drop assertion is order-dependent.
waitForsucceeds the instant the count reaches 2, so a forged event recorded even slightly later would not fail the test — the negative assertion relies entirely on flush ordering. Re-reading once after the wait settles (or asserting on a per-entity query forpkg-not-a-dependency) makes this security-relevant behavior actually pinned.🤖 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/static-call-metering.workers.test.ts` around lines 351 - 364, The forged-event assertion in the static call metering test is order-dependent because waitFor can finish before the forged event is processed. After the rollup wait settles, perform a follow-up read or per-entity query for “pkg-not-a-dependency” and assert that the forged event was not recorded, while preserving the existing granted-event count assertions.packages/worker/src/mcp/run-kody-registry.ts (1)
1063-1070: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAwaited flush has no timeout — a stalled bridge holds the run open.
The
finallyawaitsflush()unbounded, so a slow/hung metering RPC extends every run until the executor timeout. This contradicts the never-block-the-observed-path contract stated for run records (Line 826). Bounding it keeps the semantics (flush while dispatchers are live) without letting metering own the run's tail latency.Note also that events reported by promises settling after the entrypoint resolves land after the flush and are silently lost — worth stating in
usage-metering.mdalongside the overflow-drop note.🔧 Proposed fix
if (typeof __kodyStaticCallMeter !== 'undefined' && __kodyStaticCallMeter != null) { - try { await __kodyStaticCallMeter.flush(); } catch {} + try { + await Promise.race([ + __kodyStaticCallMeter.flush(), + new Promise((resolve) => setTimeout(resolve, 2000)), + ]); + } catch {} }🤖 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-kody-registry.ts` around lines 1063 - 1070, Bound the await of __kodyStaticCallMeter.flush() in the finally block of the run flow so a stalled metering bridge cannot hold the run open, while retaining the best-effort flush and swallowing timeout or flush errors. Document in usage-metering.md that events from promises settling after entrypoint completion occur after the final flush and are therefore dropped, alongside the existing overflow-drop behavior.
🤖 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/package-runtime/runtime-source-modules.ts`:
- Around line 265-273: Replace the instanceof Promise check in the
result-handling flow with thenable detection by validating that result is
non-null and has a callable then property. Attach success and error settlement
callbacks through that then method, return the original result, and keep
synchronous success recording only for non-thenable values.
In `@packages/worker/src/usage/package-static-call-usage.ts`:
- Line 36: Reduce the maxEventsPerBatch cap from 1000 to a value below the 250
writeDataPoint limit, leaving headroom for other usage writes in the same
invocation. Preserve the existing batching behavior and per-event writes while
ensuring a batch cannot exhaust the Analytics Engine write allowance.
---
Nitpick comments:
In `@packages/worker/src/mcp/run-kody-registry.ts`:
- Around line 1063-1070: Bound the await of __kodyStaticCallMeter.flush() in the
finally block of the run flow so a stalled metering bridge cannot hold the run
open, while retaining the best-effort flush and swallowing timeout or flush
errors. Document in usage-metering.md that events from promises settling after
entrypoint completion occur after the final flush and are therefore dropped,
alongside the existing overflow-drop behavior.
In `@packages/worker/src/package-runtime/module-graph.node.test.ts`:
- Around line 821-824: Update the named-export assertion in the module-graph
test to match the complete emitted export statement, including the generated
local export expression and its alias “add,” rather than asserting only the
generic substring “as add.” Keep the existing proxy generation assertion
unchanged.
In `@packages/worker/src/package-runtime/static-call-metering.workers.test.ts`:
- Around line 351-364: The forged-event assertion in the static call metering
test is order-dependent because waitFor can finish before the forged event is
processed. After the rollup wait settles, perform a follow-up read or per-entity
query for “pkg-not-a-dependency” and assert that the forged event was not
recorded, while preserving the existing granted-event count assertions.
🪄 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: 5bcb9023-f326-4780-ba02-4ff2979d18ff
📒 Files selected for processing (13)
docs/contributing/architecture/usage-metering.mdpackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/mcp/run-kody-registry.tspackages/worker/src/mcp/runtime-helper-manifest.tspackages/worker/src/package-runtime/module-export-names.node.test.tspackages/worker/src/package-runtime/module-export-names.tspackages/worker/src/package-runtime/module-graph-import-rewriting.tspackages/worker/src/package-runtime/module-graph.node.test.tspackages/worker/src/package-runtime/runtime-source-modules.tspackages/worker/src/package-runtime/static-call-metering.workers.test.tspackages/worker/src/usage/package-static-call-usage.node.test.tspackages/worker/src/usage/package-static-call-usage.tspackages/worker/src/usage/record-usage.ts
| if (result instanceof Promise) { | ||
| result.then( | ||
| () => __kodyRecordStaticPackageCall(packageId, startedAtMs, 'success'), | ||
| () => __kodyRecordStaticPackageCall(packageId, startedAtMs, 'error'), | ||
| ); | ||
| return result; | ||
| } | ||
| __kodyRecordStaticPackageCall(packageId, startedAtMs, 'success'); | ||
| return result; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
instanceof Promise misses thenables and cross-realm promises.
Exports returning a custom thenable (or a promise from another realm — bundled artifact modules can be evaluated in a different module context) get recorded as success synchronously, with durationMs excluding settlement and later rejections counted as successes. Duck-typing then matches the documented "through settlement" semantics.
🔧 Proposed fix
- if (result instanceof Promise) {
- result.then(
+ if (result != null && typeof result.then === 'function') {
+ Promise.resolve(result).then(
() => __kodyRecordStaticPackageCall(packageId, startedAtMs, 'success'),
() => __kodyRecordStaticPackageCall(packageId, startedAtMs, 'error'),
);
return result;
}📝 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.
| if (result instanceof Promise) { | |
| result.then( | |
| () => __kodyRecordStaticPackageCall(packageId, startedAtMs, 'success'), | |
| () => __kodyRecordStaticPackageCall(packageId, startedAtMs, 'error'), | |
| ); | |
| return result; | |
| } | |
| __kodyRecordStaticPackageCall(packageId, startedAtMs, 'success'); | |
| return result; | |
| if (result != null && typeof result.then === 'function') { | |
| Promise.resolve(result).then( | |
| () => __kodyRecordStaticPackageCall(packageId, startedAtMs, 'success'), | |
| () => __kodyRecordStaticPackageCall(packageId, startedAtMs, 'error'), | |
| ); | |
| return result; | |
| } | |
| __kodyRecordStaticPackageCall(packageId, startedAtMs, 'success'); | |
| return result; |
🤖 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/runtime-source-modules.ts` around lines
265 - 273, Replace the instanceof Promise check in the result-handling flow with
thenable detection by validating that result is non-null and has a callable then
property. Attach success and error settlement callbacks through that then
method, return the original result, and keep synchronous success recording only
for non-thenable values.
There was a problem hiding this comment.
Intentional, and now documented in the code (46290ea): subscribing to an arbitrary user thenable would invoke its then() from the metering path, which can trigger lazy side effects — query-builder-style thenables execute work when first awaited, so Promise.resolve(result).then(...) could run user work the caller never requested. The wrapper's hard rule is that it must never change call-path behavior, and that outranks settlement-accurate durations for exotic return values. Cross-realm promises don't arise here: all bundle modules (including composed published artifacts) evaluate in one isolate/module context, so async exports return same-realm native promises. Non-promise thenables record at return time, which is the safe degradation.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
…gling-promise limit Async calls that settle while a flush RPC is in flight now get delivered by a follow-up flush round; calls still pending when the run's entrypoint finishes remain unmetered by design (metering never extends a run's lifetime). Addresses the Bugbot review on PR #1047.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d14159f. Configure here.
| const events = __kodyStaticCallMeterEvents.splice(0, __kodyStaticCallMeterEvents.length); | ||
| await ${staticCallMeterRuntimeBridgeProviderName}.record({ events }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Flush drops remaining buffered events
Medium Severity
The sandbox flush helper stops after three bridge rounds and returns immediately when the buffer is empty between rounds. Static calls whose metering report runs during a later in-flight round (or after the third) can remain in the buffer when flush finishes, so those calls consume the per-run cap but never become package_static_call usage on the host.
Reviewed by Cursor Bugbot for commit d14159f. Configure here.
There was a problem hiding this comment.
By design, and a restatement of the delivery-window limit already documented in usage-metering.md and the prelude comment: events reported after the bounded flush window (calls whose promises settle later than the entrypoint plus in-flight flush rounds) are intentionally not delivered — metering never extends a run's lifetime, and the whole flush is additionally capped by the wrapper's 2s race. The round bound must stay finite: an unbounded "loop until empty" could be held open indefinitely by a pathological stream of late-settling promises, which is exactly the tail-latency ownership the previous review round asked us to prevent. The cap-slot consumption by undelivered events is harmless — the cap exists to bound the Analytics Engine write budget, and undelivered events consume none of it.
Stamp the callee saved-package id into the bundler-generated static import proxy, wrap function-valued exports (default + statically discovered named exports) in a transparent call-metering Proxy, and report one non-blocking usage event per call through a sandbox->host runtime bridge. The host validates the stamped id against the same bundler-provenance grant set as packageStorage before recording, and drops mismatches silently.
Per-call fire-and-forget RPCs race dispatcher teardown (rejected with 'Execution has already completed'), so each metered call now does one synchronous buffer push and the run wrapper flushes the batch over the bridge while the dispatchers are still live. Adds node + workers E2E coverage and documents the event type in usage-metering.md.
…gling-promise limit Async calls that settle while a flush RPC is in flight now get delivered by a follow-up flush round; calls still pending when the run's entrypoint finishes remain unmetered by design (metering never extends a run's lifetime). Addresses the Bugbot review on PR #1047.
Analytics Engine allows 250 writeDataPoint calls per invocation, so the per-run cap (enforced cumulatively sandbox-side and host-side) leaves headroom for the run's other usage events. The wrapper's flush await is raced against a short timeout so a slow metering bridge never owns the run's tail latency. Pins Proxy construct passthrough with a workers test (new on a wrapped class) and tightens review-flagged assertions.
…y note - Validate stamped static-call ids against the bundle's static dependency package ids only (strictly tighter than the packageStorage grant set, which also includes the run's own package id and dynamic-import deps); code comments and usage-metering.md now state the exact set. - Surface package_static_call on admin usage pages (curated metric list, AdminUsageMetric union, admin-user-usage capability schema, chart series with a new teal palette entry). - Document that the 200-events-per-run cap shares the ~250 writeDataPoint-per-invocation Analytics Engine budget with other usage events like outbound_fetch; aggregation is the eventual fix.
d14159f to
20353bf
Compare
execute-typecheck startup-budget tests and the disaster-recovery wrangler contract test timed out on a loaded runner; both pass locally on this head in seconds.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/package-runtime/static-call-metering.workers.test.ts (1)
126-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a shared exported constant for the package static-call metric key.
'package_static_call'appears in both the test SQL and the usage/record/admin type lists; exporting it from one usage definition would prevent this query from drifting away from the source of truth if the key is renamed.🤖 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/static-call-metering.workers.test.ts` around lines 126 - 138, The readStaticCallRollup query should use a shared exported constant for the package static-call metric key instead of the hardcoded 'package_static_call' string. Export the constant from the canonical usage definition, import it into this test, and interpolate or bind it in the metric filter so all references remain synchronized.
🤖 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/package-runtime/static-call-metering.workers.test.ts`:
- Around line 126-138: The readStaticCallRollup query should use a shared
exported constant for the package static-call metric key instead of the
hardcoded 'package_static_call' string. Export the constant from the canonical
usage definition, import it into this test, and interpolate or bind it in the
metric filter so all references remain synchronized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae7b20f2-f294-42fe-8693-1009562c46cc
📒 Files selected for processing (18)
docs/contributing/architecture/usage-metering.mdpackages/worker/client/charts/chart-theme.tspackages/worker/client/charts/usage-metric-series.tspackages/worker/src/admin/user-usage-data.tspackages/worker/src/app/loader-data.tspackages/worker/src/mcp/capabilities/admin/admin-user-usage.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/mcp/run-kody-registry.tspackages/worker/src/mcp/runtime-helper-manifest.tspackages/worker/src/package-runtime/module-export-names.node.test.tspackages/worker/src/package-runtime/module-export-names.tspackages/worker/src/package-runtime/module-graph-import-rewriting.tspackages/worker/src/package-runtime/module-graph.node.test.tspackages/worker/src/package-runtime/runtime-source-modules.tspackages/worker/src/package-runtime/static-call-metering.workers.test.tspackages/worker/src/usage/package-static-call-usage.node.test.tspackages/worker/src/usage/package-static-call-usage.tspackages/worker/src/usage/record-usage.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/src/usage/record-usage.ts
- docs/contributing/architecture/usage-metering.md
|
Conductor production verification ✅ (deployed in f0d0b2b): |
3 participants
What
Static imports (
import fn from 'kody:@scope/pkg/export') are the default for package reuse, but calls through them were invisible to usage metering. This PR adds a new member of the closedUsageEventTypeunion,package_static_call, and emits one non-blocking usage event per call of a statically imported, function-valued package export:userId= the calling user (host-side, from caller context)entityId= the callee package id (the package whose export was imported)durationMs= wall time of the call (through settlement for async functions)outcome=success|error(whether the wrapped call threw/rejected — the error still propagates to the caller unchanged)The metric also renders on the admin usage pages (curated
adminUsageMetricslist, insights + per-user charts).How
ensurePackageProxyknows which saved package akody:@…specifier resolved to; it now emits a metered proxy (createMeteredPackageImportProxySource) that bakes the callee package id in and wraps function-valued exports (default + statically discovered named exports) in a transparentProxyfrom the virtual runtime module. Only[[Call]]is trapped, sonewon a wrapped class constructs the real class.export * fromstays as a passthrough, so any export name the bundler cannot statically discover keeps working (it is just unmetered). Non-function exports pass through unchanged; dynamic-import proxies and root self-imports keep the plain proxy.module-export-names.tscollects runtime export names (Babel AST, skipping type-only exports, following relativeexport * fromchains inside the file map). Missing a name is always safe (passthrough).writeDataPoint-per-invocation budget); the run wrapper inrunBundledModuleWithRegistryflushes the batch over a new runtime-helper bridge while the sandbox RPC dispatchers are still live (a fire-and-forget RPC per call races dispatcher teardown and loses events — verified empirically), bounded by a short timeout so metering never owns the run's tail latency. The host (createPackageStaticCallMeterTools) only records events whose stamped id is one of the bundle's static dependency package ids recorded at build time — a strict subset of thepackageStorage()grant set, which additionally grants the run's own package id and dynamic-import dependencies, neither of which the bundler ever stamps into a metered proxy. Everything else is silently dropped. Trust model stated honestly in code + docs: generated code still runs in the sandbox realm, so forgery can at worst inflate counts for packages the bundle already statically depends on.recordUsage); flush failures are swallowed.Testing
module-export-names.node.test.ts— declaration/specifier/destructuring/type-only/star-chain/cycle/parse-failure cases.package-static-call-usage.node.test.ts— one event per batch entry with correctentityId/outcome/durationMs, forged/unknown ids dropped, invalid outcomes dropped, durations normalized, cumulative per-run cap, never-throws.static-call-metering.workers.test.ts— real bundles throughrunBundledModuleWithRegistryagainst local D1 rollups: 5 calls ⇒event_count: 5witherror_count: 1; a throwing export recordserrorand still throws to the caller;newon a wrapped class constructs correctly and is not metered; non-function exports untouched; published-artifact imports metered; an in-sandbox forged stamp for a non-dependency package records nothing (with a post-settle re-check).npm run validategreen locally (fresh Playwright E2E run: 19 passed); re-validated after rebasing ontomain@c54a2686(lean-invoke + deprecation shims) — all 138 overlapping unit test files pass post-rebase.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@c54a2686· Head:a10ac251Classification: extends — no new primitives; the package runtime's import rewriting, the execute runtime wrapper, the runtime-helper bridge surface, usage metering, and the admin usage read model each gain behavior.
Primitives touched
package-runtimecapabilities-executemcp-serverstaticCallMeter) in the helper manifestusage-meteringpackage_static_callevent type + validated host-side sinkapp-uipackage_static_calladded to admin usage chart series (new teal color)rbacSystem map
A statically imported package export call flows from the sandbox call-metering wrapper through the run-end bridge flush into validated usage events, which the admin usage pages now chart.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
Per-user isolation holds: events are attributed to the calling user only, and stamped callee ids are validated host-side against the bundle's static dependency ids recorded at build time for that user's own bundle — sandbox-supplied ids can never attribute usage across users or to non-dependency packages.
Program report
Status: MERGED + DEPLOY-VERIFIED (agent side).
f0d0b2b89bcec1cab35b1775ac6d94a33d088d87.main@f0d0b2b8: success (run 30541425455).f0d0b2b8: success (run 30541718572).https://heykody.dev/health→{"ok":true,"commitSha":"f0d0b2b89bcec1cab35b1775ac6d94a33d088d87"}— matches the merge commit.kody_usage_eventsforpackage_static_callrows (blobs:[userId, eventType, entityId, outcome, timestamp], doubles:[durationMs, cpuMs, bytes]) — and confirm here.main@c54a2686(lean-invoke + deprecation shims), no semantic conflicts; CI fully green on heada10ac251(one Node round flaked on runner load in files outside this diff, retriggered and green). QA follow-ups all landed: static-deps-only validation set,package_static_callon admin usage pages, Analytics Engine capacity note. Reviewer findings triaged: two fixed (bounded flush rounds; cumulative 200-event cap for the AE budget), three declined with evidence (Proxy construct claim — pinned by test; thenable subscription — lazy side-effect risk; late-settling-events rehash — documented delivery-window limit), one low-value nitpick skipped (shared constant for the metric key; string literals with the closed union are the codebase pattern).package_static_callrows inkody_usage_events.Summary by CodeRabbit
package_static_callevent type, validation rules, and nesting guidance.