Conversation
copyBounded mixed six concerns in one 120-line body: the depth guard, leaf passthrough, un-awaited promise rejection, intra-sandbox host wrapping, boundary serialization and container recursion. Every value type was matched by a hand-written instanceof chain, so adding one meant editing three different chains and the function grew a return statement each time. Replace the chains with ordered lookup tables (HOST_VALUE_WRAPPERS, BOUNDARY_SERIALIZERS) dispatched by a shared matchType helper, and extract isDataLeaf, isSandboxValue, hasNoJsonForm, serializeForBoundary and copyContainer. Ordering within each table preserves the original chain order, which is what makes this behavior-preserving; the sandbox value classes are standalone rather than subclasses of their host counterparts, so no value matches two entries. Qlty (packages/codemode/src/tool-runtime.ts): before: complexity 68, 15 returns, 3 complex binary expressions (185/216/257) after: complexity 19, returns and binary-expression smells cleared file total complexity 266 -> 243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The suite reached copyIn only indirectly through CodeMode.execute, which never
drives a host Map/Set/RegExp/URL through an intra-sandbox checkpoint, leaving
the wrapper branches uncovered.
Add 18 characterization tests over both copyIn modes: leaf passthrough,
boundary serialization of Date/URL to strings and of the JSON-formless types
to {}, intra-sandbox identity passthrough and host wrapping, and every
contract violation (circular, depth limit and just inside it, blocked
properties, non-plain objects, non-data leaves).
These were written against the pre-refactor implementation and pass unchanged
on both it and the refactored one, so they pin behavior rather than describing
the new structure.
packages/codemode/src/tool-runtime.ts line coverage 99.63% -> 99.64%; the two
remaining uncovered lines predate this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
turbo.json only registers test tasks for the packages a change touches, and codemode had no entry, so packages/codemode/test never ran on push despite having its own test script. Add @opencode-ai/codemode#test so the tests covering the copyBounded refactor actually run in CI. The suite is 281 tests in ~1.4s, so this is a negligible addition to the unit job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
1. Issue
Link to the associated GitHub issue:
#93
Full path to the refactored file:
packages/codemode/src/tool-runtime.ts
What do you think this file does?
This is the layer that sits between the host and the sandbox when CodeMode runs tools. Anything crossing it has to be plain data, so it checks each value before letting it through: depth, circularity, plain-object prototypes and blocked property names. It also converts value types (Date, RegExp, Map, Set, URL, URLSearchParams) between their host and sandbox forms.
What is the scope of your refactoring within that file?
Only
copyBounded(lines 174-294), the recursive helper behind the exportedcopyIn. No other function in the file was touched.Which Qlty-reported issue did you address?
Function with high complexity (count = 68): copyBounded at line 174, plus Function with many returns (count = 15) on the same function and three Complex binary expression smells inside it (lines 185, 216, 257).
2. Refactoring
How did the specific issue you chose impact the codebase's maintainability?
copyBounded packed six concerns into one 120-line body and matched every value type with hand-written instanceof chains, of which there were three. Adding one value type meant editing all three chains in the right order plus another return, which is how it reached 15 returns and complexity 68.
What changes did you make to resolve the issue?
I replaced the chains with two ordered lookup tables (HOST_VALUE_WRAPPERS, BOUNDARY_SERIALIZERS) dispatched by a shared matchType helper, and extracted isDataLeaf, isSandboxValue, hasNoJsonForm, isoOrNull, serializeForBoundary and copyContainer. Each table keeps the original chain order, which is what makes the change behavior-preserving.
How do your changes improve maintainability? Did you consider alternatives?
Adding a value type is now one row in one table instead of three chain edits, and copyBounded reads as its six phases in sequence (complexity 68 to 19, returns 15 to 5, binary-expression smells 3 to 0, file total 266 to 243). I first tried extracting each chain into its own function, but that only relocates the smells: a helper wrapping the 7-branch chain still has 7 returns (Qlty flags at 6), so table dispatch was the version that actually cleared them.
3. Validation
How did you validate that the change is correct?
All 263 pre-existing codemode tests still pass, and I ran 60 inputs through the original and refactored copyIn and diffed the results (both modes across every value type, nesting, cycles, depth limits and error paths), which came out byte-identical. I then added 18 tests in packages/codemode/test/tool-runtime.test.ts covering both dispatch tables and every error path.
Which tests cover the change, and why are they sufficient?
packages/codemode/test/tool-runtime.test.ts plus the existing suite, 281 tests total. They are sufficient because the new tests were written against the pre-refactor code and pass unchanged on both versions, so they pin observable behavior rather than the new structure. Running only the new test file, the uncovered lines of tool-runtime.ts jump from 143-148 straight to 346-361, and the refactored region is lines 174-345, so the new tests alone execute every changed line. They also close a real gap, since the old suite never drove a host Map or Set through an intra-sandbox checkpoint.
Line coverage of the file went from 99.63% to 99.64% (the two remaining uncovered lines predate this PR).
bun run typecheckis clean.Note on
bun lint: opencode reports 698 warnings and 2 errors on a cleanmain, so it cannot pass for anyone. This branch reports exactly the same 698 and 2, meaning this PR introduces no new lint findings, and linting only the two files this PR changes reports 0 warnings and 0 errors. The screenshot below shows all three.Note on CI:
turbo.jsononly registers test tasks for listed packages and codemode had none, so its tests never ran on push. This PR adds@opencode-ai/codemode#test, and the CI screenshot shows the new tests running by name inside the unit job (281 tests, 10 tasks successful).Attach a screenshot of the test coverage showing the lines were executed by the tests.