refactor(schema): use Zod transforms instead of manual mutation #199

boneskull · 2025-12-18T01:08:55Z

Replace manual result mutation in safeParseConfig/safeParsePartialConfig
with proper Zod .transform() methods:

Add budgetSchema transform for string-to-number conversion (e.g., '10ms' → ns)
Add budgetsSchema transform for nested-to-flat structure conversion
Extract baseConfigProperties to avoid duplication between runtime and
JSON Schema generation schemas
Export ModestBenchConfigInput type for consumers needing the input shape
Export partialModestBenchConfigInputSchema for JSON Schema generation
(transforms can't be represented in JSON Schema)

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]

boneskull · 2025-12-18T01:09:13Z

refactor(schema): use Zod transforms instead of manual mutation #199 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copilot

Pull request overview

This PR refactors the ModestBench configuration schema to use Zod's built-in transform functionality instead of manual mutation. The changes improve type safety by properly distinguishing between input and output types, eliminate the need for manual post-parse transformations, and ensure JSON Schema generation works correctly.

Key changes:

Replaced manual budget transformation with declarative Zod .transform() methods
Introduced separate input/output schemas to support JSON Schema generation (transforms cannot be represented in JSON Schema)
Extracted shared configuration properties to avoid duplication between runtime and JSON Schema schemas

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/config/schema.ts	Core refactoring: added budgetSchema and budgetsSchema transforms, extracted baseConfigProperties for reuse, created separate input/output schemas, exported ModestBenchConfigInput type
src/services/config-manager.ts	Removed unnecessary type assertion now that schema properly types the transformation
src/types/core.ts	Added ModestBenchConfigInput export and TODO comment for ThresholdConfig; includes duplicate export that should be removed
scripts/generate-schema.ts	Updated to use partialModestBenchConfigInputSchema (without transforms) for JSON Schema generation

Comments suppressed due to low confidence (1)

src/types/core.ts:308

Duplicate export statement. The functions createRunId and createTaskId are already exported at line 127. This duplicate export at line 307-308 should be removed to avoid redundancy.

/**
 * Reporter-specific configuration

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

src/services/config-manager.ts

cloudflare-workers-and-pages · 2025-12-18T01:27:46Z

Deploying modestbench with Cloudflare Pages

Latest commit:	`c4f94e1`
Status:	✅ Deploy successful!
Preview URL:	https://2e2c3199.modestbench.pages.dev
Branch Preview URL:	https://feature-modestbench-parse-fi.modestbench.pages.dev

View logs

Replace manual result mutation in safeParseConfig/safeParsePartialConfig with proper Zod .transform() methods: - Add budgetSchema transform for string-to-number conversion (e.g., '10ms' → ns) - Add budgetsSchema transform for nested-to-flat structure conversion - Extract baseConfigProperties to avoid duplication between runtime and JSON Schema generation schemas - Export ModestBenchConfigInput type for consumers needing the input shape - Export partialModestBenchConfigInputSchema for JSON Schema generation (transforms can't be represented in JSON Schema) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Copilot

Pull request overview

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

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

Copilot · 2025-12-18T01:34:43Z

src/config/schema.ts

+  return {
+    // Build absolute budget object if present
+    absolute: budget.absolute
+      ? {
+          maxP99:
+            budget.absolute.maxP99 !== undefined
+              ? typeof budget.absolute.maxP99 === 'string'
+                ? parseTimeString(budget.absolute.maxP99)
+                : budget.absolute.maxP99
+              : undefined,
+          maxTime:
+            budget.absolute.maxTime !== undefined
+              ? typeof budget.absolute.maxTime === 'string'
+                ? parseTimeString(budget.absolute.maxTime)
+                : budget.absolute.maxTime
+              : undefined,
+          minOpsPerSec: budget.absolute.minOpsPerSec,
+        }
+      : undefined,
+    // Build relative budget object if present
+    relative: budget.relative
+      ? {
+          maxRegression:
+            budget.relative.maxRegression !== undefined
+              ? typeof budget.relative.maxRegression === 'string'
+                ? parsePercentageString(budget.relative.maxRegression)
+                : budget.relative.maxRegression
+              : undefined,


The transform creates an absolute object even when all its properties are undefined. For example, if budget.absolute is {}, the result will be { absolute: { maxP99: undefined, maxTime: undefined, minOpsPerSec: undefined } } instead of { absolute: undefined }.

This could cause issues when the budget is later used, as code might check if (budget.absolute) expecting it to be falsy when no absolute thresholds are set, but it will be an object with all undefined properties.

Consider checking if all properties are undefined and returning undefined for the absolute property itself in that case, similar to how the relative budget object should be handled.

Suggested change

return {

// Build absolute budget object if present

absolute: budget.absolute

? {

maxP99:

budget.absolute.maxP99 !== undefined

? typeof budget.absolute.maxP99 === 'string'

? parseTimeString(budget.absolute.maxP99)

: budget.absolute.maxP99

: undefined,

maxTime:

budget.absolute.maxTime !== undefined

? typeof budget.absolute.maxTime === 'string'

? parseTimeString(budget.absolute.maxTime)

: budget.absolute.maxTime

: undefined,

minOpsPerSec: budget.absolute.minOpsPerSec,

}

: undefined,

// Build relative budget object if present

relative: budget.relative

? {

maxRegression:

budget.relative.maxRegression !== undefined

? typeof budget.relative.maxRegression === 'string'

? parsePercentageString(budget.relative.maxRegression)

: budget.relative.maxRegression

: undefined,

// Transform absolute budget values

const absoluteMaxP99 =

budget.absolute?.maxP99 !== undefined

? typeof budget.absolute.maxP99 === 'string'

? parseTimeString(budget.absolute.maxP99)

: budget.absolute.maxP99

: undefined;

const absoluteMaxTime =

budget.absolute?.maxTime !== undefined

? typeof budget.absolute.maxTime === 'string'

? parseTimeString(budget.absolute.maxTime)

: budget.absolute.maxTime

: undefined;

const absoluteMinOpsPerSec = budget.absolute?.minOpsPerSec;

const hasAbsolute =

absoluteMaxP99 !== undefined ||

absoluteMaxTime !== undefined ||

absoluteMinOpsPerSec !== undefined;

// Transform relative budget values

const relativeMaxRegression =

budget.relative?.maxRegression !== undefined

? typeof budget.relative.maxRegression === 'string'

? parsePercentageString(budget.relative.maxRegression)

: budget.relative.maxRegression

: undefined;

const hasRelative = relativeMaxRegression !== undefined;

return {

// Build absolute budget object only if any field is defined

absolute: hasAbsolute

? {

maxP99: absoluteMaxP99,

maxTime: absoluteMaxTime,

minOpsPerSec: absoluteMinOpsPerSec,

}

: undefined,

// Build relative budget object only if any field is defined

relative: hasRelative

? {

maxRegression: relativeMaxRegression,

Copilot · 2025-12-18T01:34:43Z

src/config/schema.ts

+  return {
+    // Build absolute budget object if present
+    absolute: budget.absolute
+      ? {
+          maxP99:
+            budget.absolute.maxP99 !== undefined
+              ? typeof budget.absolute.maxP99 === 'string'
+                ? parseTimeString(budget.absolute.maxP99)
+                : budget.absolute.maxP99
+              : undefined,
+          maxTime:
+            budget.absolute.maxTime !== undefined
+              ? typeof budget.absolute.maxTime === 'string'
+                ? parseTimeString(budget.absolute.maxTime)
+                : budget.absolute.maxTime
+              : undefined,
+          minOpsPerSec: budget.absolute.minOpsPerSec,
+        }
+      : undefined,
+    // Build relative budget object if present
+    relative: budget.relative
+      ? {
+          maxRegression:
+            budget.relative.maxRegression !== undefined
+              ? typeof budget.relative.maxRegression === 'string'
+                ? parsePercentageString(budget.relative.maxRegression)
+                : budget.relative.maxRegression
+              : undefined,
+        }
+      : undefined,


The transform creates a relative object even when all its properties are undefined. For example, if budget.relative is {}, the result will be { relative: { maxRegression: undefined } } instead of { relative: undefined }.

This could cause issues when the budget is later used, as code might check if (budget.relative) expecting it to be falsy when no relative thresholds are set, but it will be an object with undefined properties.

Consider checking if all properties are undefined and returning undefined for the relative property itself in that case.

Suggested change

return {

// Build absolute budget object if present

absolute: budget.absolute

? {

maxP99:

budget.absolute.maxP99 !== undefined

? typeof budget.absolute.maxP99 === 'string'

? parseTimeString(budget.absolute.maxP99)

: budget.absolute.maxP99

: undefined,

maxTime:

budget.absolute.maxTime !== undefined

? typeof budget.absolute.maxTime === 'string'

? parseTimeString(budget.absolute.maxTime)

: budget.absolute.maxTime

: undefined,

minOpsPerSec: budget.absolute.minOpsPerSec,

}

: undefined,

// Build relative budget object if present

relative: budget.relative

? {

maxRegression:

budget.relative.maxRegression !== undefined

? typeof budget.relative.maxRegression === 'string'

? parsePercentageString(budget.relative.maxRegression)

: budget.relative.maxRegression

: undefined,

}

: undefined,

// Build absolute budget object if present

const absolute = budget.absolute

? {

maxP99:

budget.absolute.maxP99 !== undefined

? typeof budget.absolute.maxP99 === 'string'

? parseTimeString(budget.absolute.maxP99)

: budget.absolute.maxP99

: undefined,

maxTime:

budget.absolute.maxTime !== undefined

? typeof budget.absolute.maxTime === 'string'

? parseTimeString(budget.absolute.maxTime)

: budget.absolute.maxTime

: undefined,

minOpsPerSec: budget.absolute.minOpsPerSec,

}

: undefined;

// Build relative budget object if present; ensure it's undefined when all properties are undefined

let relative: Budget['relative'];

if (budget.relative) {

const maxRegression =

budget.relative.maxRegression !== undefined

? typeof budget.relative.maxRegression === 'string'

? parsePercentageString(budget.relative.maxRegression)

: budget.relative.maxRegression

: undefined;

// Only create the relative object if at least one property is defined

if (maxRegression !== undefined) {

relative = { maxRegression };

} else {

relative = undefined;

}

} else {

relative = undefined;

}

return {

absolute,

relative,

Copilot · 2025-12-18T01:34:43Z

src/config/schema.ts

+const flattenBudgets = (
+  nested: z.infer<typeof budgetsInputSchema>,
+): Record<string, Budget> => {
+  const flat: Record<string, Budget> = {};

-    // Copy minOpsPerSec as-is (already a number)
-    if (budget.absolute.minOpsPerSec !== undefined) {
-      transformed.absolute.minOpsPerSec = budget.absolute.minOpsPerSec;
-    }
-
-    // Parse time strings
-    if (budget.absolute.maxTime !== undefined) {
-      transformed.absolute.maxTime =
-        typeof budget.absolute.maxTime === 'string'
-          ? parseTimeString(budget.absolute.maxTime)
-          : budget.absolute.maxTime;
-    }
-    if (budget.absolute.maxP99 !== undefined) {
-      transformed.absolute.maxP99 =
-        typeof budget.absolute.maxP99 === 'string'
-          ? parseTimeString(budget.absolute.maxP99)
-          : budget.absolute.maxP99;
+  for (const [file, suites] of Object.entries(nested)) {
+    for (const [suite, tasks] of Object.entries(suites)) {
+      for (const [task, budget] of Object.entries(tasks)) {
+        const taskId = `${file}/${suite}/${task}`;
+        flat[taskId] = budget;
+      }
    }
  }

-  if (budget.relative) {
-    transformed.relative = {};
+  return flat;
+};


The budget transformation logic introduced in this PR lacks comprehensive test coverage. The existing tests in test/unit/config/budget-config-integration.test.ts verify that parsing succeeds, but don't verify the exact shape of the transformed output.

Consider adding tests that verify:

Budgets with only some properties set don't create objects with all-undefined properties

String values like "10ms" are correctly transformed to nanoseconds

Percentage strings like "15%" are correctly transformed to decimals

The nested file→suite→task structure is correctly flattened to taskId keys

These tests would help catch the issues with empty object creation and ensure the transforms behave correctly.

Copilot AI review requested due to automatic review settings December 18, 2025 01:08

github-actions bot assigned boneskull Dec 18, 2025

Copilot started reviewing on behalf of boneskull December 18, 2025 01:09 View session

boneskull force-pushed the feature/modestbench-parse-fix branch from eadd55f to 3fbe439 Compare December 18, 2025 01:11

Copilot AI reviewed Dec 18, 2025

View reviewed changes

src/services/config-manager.ts Outdated Show resolved Hide resolved

boneskull force-pushed the feature/modestbench-parse-fix branch from 3fbe439 to 6e02506 Compare December 18, 2025 01:26

Copilot AI review requested due to automatic review settings December 18, 2025 01:29

boneskull force-pushed the feature/modestbench-parse-fix branch from 6e02506 to c4f94e1 Compare December 18, 2025 01:29

Copilot started reviewing on behalf of boneskull December 18, 2025 01:29 View session

boneskull merged commit 6f17059 into main Dec 18, 2025
15 checks passed

boneskull deleted the feature/modestbench-parse-fix branch December 18, 2025 01:31

Copilot AI reviewed Dec 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(schema): use Zod transforms instead of manual mutation #199

refactor(schema): use Zod transforms instead of manual mutation #199

Uh oh!

boneskull commented Dec 18, 2025

Uh oh!

boneskull commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

cloudflare-workers-and-pages bot commented Dec 18, 2025 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

refactor(schema): use Zod transforms instead of manual mutation #199

refactor(schema): use Zod transforms instead of manual mutation #199

Uh oh!

Conversation

boneskull commented Dec 18, 2025

Uh oh!

boneskull commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

cloudflare-workers-and-pages bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying modestbench with Cloudflare Pages

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cloudflare-workers-and-pages bot commented Dec 18, 2025 •

edited

Loading