Skip to content

feat(vm): add maxSteps execution budget#2

Open
Dayifour wants to merge 6 commits into
darylcecile:mainfrom
Dayifour:feat/vm-max-steps-budget
Open

feat(vm): add maxSteps execution budget#2
Dayifour wants to merge 6 commits into
darylcecile:mainfrom
Dayifour:feat/vm-max-steps-budget

Conversation

@Dayifour
Copy link
Copy Markdown

@Dayifour Dayifour commented May 4, 2026

Add maxSteps execution budget to VM API

Summary

  • expose maxSteps in executionRules and per-call options
  • wire step budget into evaluation contexts and handle calls
  • document step budgets and add VM tests for step exhaustion

Testing

  • bun test
  • bun run typecheck

@Dayifour
Copy link
Copy Markdown
Author

Dayifour commented May 4, 2026

@darylcecile Ready for review!

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a cooperative maxSteps execution budget to the VM’s public API so hosts can limit interpreted work in addition to the existing wall-clock guard. It extends the VM resource-limiting model, updates docs, and adds basic regression coverage.

Changes:

  • Add maxSteps to VM execution rule types and per-call evaluation/import options.
  • Thread the step budget into evaluation, import, URL-evaluation, and handle execution contexts.
  • Document step budgets in the README and add a VM test for step exhaustion.
Show a summary per file
File Description
src/vm.ts Adds the new maxSteps API surface, normalizes it, and passes it into VM execution contexts.
test/vm.test.ts Adds coverage for step-budget exhaustion through vm.eval(...).
README.md Documents maxSteps in the VM options and resource-limits sections.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 5

Comment thread test/vm.test.ts Outdated
Comment on lines +194 to +195
expectVMFailure(
await overrideVm.eval("let i = 0; while (true) { i += 1; }", { maxSteps: 5 }),
Comment thread src/vm.ts
Comment on lines +776 to +779
const maxSteps = normalizeMaxSteps(
options.maxSteps ?? this.#executionRules.maxSteps,
);
const rootContext = this.#createEvaluationContext(timeLimit, maxSteps);
Comment thread src/vm.ts Outdated
Comment on lines +823 to +826
return this.evaluate(response.bodyText, {
sourceType: options.sourceType,
timeLimit: options.timeLimit,
maxSteps: options.maxSteps,
Comment thread src/vm.ts
if (!Number.isSafeInteger(value) || value < 0) {
throw new VMError(
VMErrorCode.VMRuntimeError,
"executionRules.maxSteps must be a non-negative safe integer.",
Comment thread README.md Outdated

Therefore time limits are guardrails, not hard CPU guarantees. A synchronous host capability can still block while it runs, and there is currently no independent memory limit, public instruction counter, scheduler, or deterministic event loop.

`executionRules.maxSteps` and per-call `evaluate(source, { maxSteps })` provide a cooperative step budget:
@darylcecile
Copy link
Copy Markdown
Owner

See suggestions from the copilot review above. Also, im not sure VMErrorCode.VMTimeoutError is the right error for maxSteps, something like VMErrorCode.VMStepsExceededError would be better

@Dayifour
Copy link
Copy Markdown
Author

Dayifour commented May 6, 2026

@darylcecile I've integrated the suggestions from Copilot and your feedback.
The new commit includes the VMStepsExceededError code and better validation for URL evaluation. I also fixed the test logic to properly check the budget override. Let me know if everything looks good to you now

@Dayifour Dayifour requested a review from Copilot May 6, 2026 12:15
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Dayifour Dayifour requested a review from Copilot May 6, 2026 21:14
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Dayifour
Copy link
Copy Markdown
Author

Hey @darylcecile, had a chance to look at the PR? Everything is up to date on my end

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (1)

src/vm.ts:1002

  • Deleting the module record from #moduleGraph on any evaluation failure means a module that throws (or has side effects before throwing) will be reloaded and re-executed on subsequent vm.import() calls, which diverges from typical ES module caching semantics and can cause repeated side effects / extra work. If the goal is specifically to allow retries after budget exhaustion, consider only deleting the record for budget-related failures (e.g. VM_STEPS_EXCEEDED_ERROR / VM_TIMEOUT_ERROR) and caching failures for other runtime errors.
      // If evaluation failed for any reason, remove the partially-created
      // module record from the graph so callers can retry with a different
      // execution budget or after a reset. This avoids permanently
      // caching exhausted/failed module records.
      graph.delete(record.specifier);
      if (record.status !== "evaluated") {
        record.status = "linked";
      }
      throw error;
  • Files reviewed: 7/7 changed files
  • Comments generated: 2

Comment thread src/vm.ts Outdated
Comment on lines +2240 to +2249
// Accept a plain object shape that carries a `code` string to preserve
// propagated VM error codes across boundaries where the original class
// identity may be lost (for example, when crossing promise boundaries).
if (typeof error === "object" && error !== null && "code" in (error as any)) {
try {
const maybeCode = (error as any).code as string | undefined;
const message = (error as any).message ?? String(error);
const details = (error as any).details ?? {};
if (typeof maybeCode === "string") {
return new VMError(maybeCode as VMErrorCode, message, details as any);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dayifour lets add a type narrowing helper here

Comment thread src/vm.ts Outdated
Comment on lines 842 to 881
@@ -795,7 +861,10 @@ export class VM {
);
}

if (maxBytes !== undefined && utf8ByteLength(response.bodyText) > maxBytes) {
if (
maxBytes !== undefined &&
utf8ByteLength(response.bodyText) > maxBytes
) {
return failure(
new VMError(
VMErrorCode.VMSecurityError,
@@ -808,6 +877,7 @@ export class VM {
return this.evaluate(response.bodyText, {
sourceType: options.sourceType,
timeLimit: options.timeLimit,
maxSteps: options.maxSteps,
});
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dayifour lets add this in

@Dayifour Dayifour requested review from Copilot and darylcecile May 19, 2026 18:25
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

README.md:1

  • Two documentation issues on the updated lines: (1) the example code block mixes tabs and spaces on the new maxSteps line, which breaks alignment in the rendered snippet; and (2) the API snippet states capabilities.executionRules exists while also listing a top-level executionRules alias—please ensure this matches the actual VMOptions shape implemented in src/vm.ts (if capabilities.executionRules is not supported, it should be removed from the README; if it is supported, the implementation should reflect it consistently).
# @catmint-fs/jsvm

Comment thread src/vm.ts
Comment on lines +1009 to 1021
record.failure = undefined;
} catch (error) {
if (record.status !== "evaluated") {
record.status = "linked";
const failure = toVMError(error);

if (isRetryableModuleFailure(failure)) {
graph.delete(record.specifier);
} else {
record.status = "failed";
record.failure = failure;
}
throw error;

throw failure;
}
Comment thread src/vm.ts
Comment on lines +2264 to +2266
if (isVMErrorRecord(error)) {
return new VMError(error.code, error.message, error.details);
}
Comment thread src/vm.ts
Comment on lines +2285 to +2294
function isVMErrorRecord(
error: unknown,
): error is { readonly code: VMErrorCode; readonly message: string; readonly details: VMErrorDetails } {
return (
isPlainObject(error) &&
isVMErrorCode(error.code) &&
typeof error.message === "string" &&
isPlainObject(error.details)
);
}
Comment thread src/vm.ts
Comment on lines +1495 to +1509
function normalizeMaxSteps(value: number | undefined): number | undefined {
if (value === undefined) {
return undefined;
}

if (!Number.isSafeInteger(value) || value < 0) {
throw new VMError(
VMErrorCode.VMRuntimeError,
"maxSteps must be a non-negative safe integer.",
{ valueType: typeof value },
);
}

return value;
}
Comment thread src/vm.ts Outdated
Comment on lines 850 to 857
// Validate maxSteps before any network request
if (options.maxSteps !== undefined) {
normalizeMaxSteps(options.maxSteps);
}

const href = normalizeEvaluateUrl(url);
const maxBytes = normalizeMaxBytes(options.maxBytes);
const response = await performHostNetworkRequest(
Comment thread README.md
interface VMOptions {
capabilities?: {
executionRules?: { timeLimit?: number };
executionRules?: { timeLimit?: number; maxSteps?: number };
Comment thread README.md
Comment on lines 149 to +150
// Also accepted as top-level aliases; top-level values override capabilities.*.
executionRules?: { timeLimit?: number };
executionRules?: { timeLimit?: number; maxSteps?: number };
@Dayifour
Copy link
Copy Markdown
Author

Hey @darylcecile, had a chance to look at the PR? Everything is up to date on my end

@darylcecile
Copy link
Copy Markdown
Owner

hey @Dayifour great job on this - however the extra formatting (likely coming from a formatter extension in your IDE or from your AI tooling) makes reviewing this PR difficult. Any chance you can clean up any extra formatting so that only relevant changes are left? This will also ensure formatting conformance across the rest of the codebase. I have an in-flight PR to add formatting rules to the repo - but for now lets clean this up and then i'll stamp LGTM 🚀

@Dayifour
Copy link
Copy Markdown
Author

Hey @darylcecile, I totally understand that extra formatting noise really does make diffs harder to follow.

I've done my best to clean it up, but I'm struggling to fully isolate where it's coming from. I'll keep investigating on my end. In the meantime, if the remaining changes are clear enough, I'm happy to wait for your formatting PR to land and align with it then. Let me know how you'd like to proceed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants