refactor: move backend-contract runtime helpers into core#533
refactor: move backend-contract runtime helpers into core#533charliecreates[bot] wants to merge 4 commits intomainfrom
Conversation
|
orrery preview: https://pr-533.orrery-c4f.pages.dev/ |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main concern is architectural: @rybosome/tspice-core now depends directly on @rybosome/tspice-backend-contract, which blurs layering and can create long-term coupling/cycle pressure. Secondarily, the core root export surface in src/index.ts is growing into an unfocused barrel; consider a dedicated spice-runtime subpath. Tests are comprehensive but packages/core/test/index.test.ts has become a monolith and would be easier to maintain if split.
Additional notes (3)
- Maintainability |
packages/core/package.json:20-25
packages/corenow has a hard dependency on@rybosome/tspice-backend-contract(both inpackage.jsonand via imports in the newspice-runtime/*files). That creates an architectural coupling (and potential for circular dependency pressure over time) where “core” cannot be used without the contract package.
If the intent of the refactor is that runtime helpers are shared by backends and other packages, it’s reasonable for them to depend on the types they operate on—but making core depend on backend-contract makes the layering ambiguous (core → contract), while other packages (e.g. tspice) already depend on backend-contract too. This increases the blast radius of changes and makes it harder to reuse @rybosome/tspice-core independently.
Consider either:
-
moving the relevant branded types (
Mat3RowMajor,Vec3,SpiceHandle, etc.) intocore(or a third@rybosome/tspice-typespackage) and havingbackend-contractdepend on that, or -
keeping
spice-runtimein a package that is explicitly allowed to depend onbackend-contract(e.g.backend-runtime), whilecorestays contract-agnostic. -
Maintainability |
packages/core/src/index.ts:81-142
packages/core/src/index.tsis becoming a large grab-bag of exports (unrelated utilities likenormalizeVirtualKernelPathalongside SPICE runtime validators/branders and handle registries). This increases the chance of accidental public API expansion and makes it harder for consumers to understand what is stable vs internal.
Given the PR goal (“move backend-contract runtime helpers into core”), it would be cleaner to introduce a dedicated subpath export (e.g. @rybosome/tspice-core/spice-runtime) and re-export from there, keeping the root index focused on core primitives.
- Maintainability |
packages/core/test/index.test.ts:1-1
The new core tests are now asserting runtime exports via dynamicimport()of the package name (@rybosome/tspice-core). This is fine, but the file has grown substantially and now mixes multiple concerns (@rybosome/tspice-coreinvariants/path normalization + Mat/Vec branding + ids/names normalization + kernel-kind utilities) in one file.
This makes failures noisier and increases runtime for a single test file; it also reduces debuggability when only one area is broken.
Summary of changes
What changed
Runtime helpers moved out of @rybosome/tspice-backend-contract
- Removed runtime exports/implementations from backend-contract (e.g.
assertGetmsgWhich,assertSpiceInt32,createSpiceHandleRegistry,normalizeBodItem, kernel-kind helpers, Mat3/Mat6/Vec branding helpers). - Updated contract docs/comments to explicitly point to runtime helpers living in
@rybosome/tspice-core.
New spice-runtime surface in @rybosome/tspice-core
- Added
packages/core/src/spice-runtime/*implementations:domains/error.ts(GETMSG_WHICH_VALUES,isGetmsgWhich,assertGetmsgWhich)shared/spice-int.ts(assertSpiceInt32,assertSpiceInt32NonNegative, bounds)shared/spice-handles.ts(createSpiceHandleRegistry)- Renamed/moved existing helpers (
kernels-utils.ts,ids-names-normalize.ts,mat3.ts,mat6.ts,vec.ts,errors.ts).
- Re-exported these helpers from
packages/core/src/index.tsso consumers can import from@rybosome/tspice-core.
Consumer updates
- Updated
backend-node,backend-wasm,backend-fake, andtspiceto import runtime helpers from@rybosome/tspice-core. - Added TS project references to
corewhere needed (e.g.backend-fake,tspice).
Tests adjusted
backend-contracttests now assert runtime helpers are not exported.coretests now cover the moved runtime helpers and kernel-kind utilities.- Deleted
backend-contract/test/kernels-utils.test.ts(migrated coverage intocore).
| export const SPICE_INT32_MIN = -0x80000000; // -2147483648 | ||
| export const SPICE_INT32_MAX = 0x7fffffff; // 2147483647 | ||
|
|
||
| /** | ||
| * Optional bounds used by runtime `SpiceInt` validators in `@rybosome/tspice-core`. | ||
| */ | ||
| export type AssertSpiceInt32Options = { | ||
| /** If provided, enforce `value >= min`. */ | ||
| min?: number; | ||
| /** If provided, enforce `value <= max`. */ | ||
| max?: number; | ||
| }; |
There was a problem hiding this comment.
The contract package now only exports SPICE_INT32_MIN/MAX and AssertSpiceInt32Options, but the JSDoc says these bounds are used by validators in @rybosome/tspice-core. That creates a subtle coupling in the opposite direction (core depending on contract at runtime), and it also makes backend-contract look like it still provides runtime helper surface.
Given the PR goal (“keep backend-contract focused on contract defs/constants”), it would be cleaner if AssertSpiceInt32Options lived in core with the validators, and backend-contract only kept the numeric constants if they are truly part of the contract.
Right now this isn’t a type error, but it’s an architectural smell: core already exports AssertSpiceInt32Options, and backend-contract exporting it too increases the chance of consumers importing the wrong one and reintroducing circular-ish dependencies.
Suggestion
Move AssertSpiceInt32Options out of @rybosome/tspice-backend-contract and into @rybosome/tspice-core (next to assertSpiceInt32). In backend-contract, keep only SPICE_INT32_MIN/MAX if you want them as contract constants.
Concretely:
- In
packages/backend-contract/src/shared/spice-int.ts, removeAssertSpiceInt32Optionsand its comment. - In
packages/backend-contract/src/index.ts, removeexport type { AssertSpiceInt32Options } .... - In
packages/core/src/spice-runtime/shared/spice-int.ts, keep/ownAssertSpiceInt32Options(already present) and ensure it’s re-exported fromcore/src/index.ts(already done). - Update any consumers still importing the type from backend-contract.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| return { | ||
| register, | ||
| lookup, | ||
| close, | ||
| size: () => handles.size, | ||
|
|
||
| // Internal hook used by the Node backend to best-effort dispose all open handles. | ||
| // Not part of the public backend contract. | ||
| __entries: () => Array.from(handles.entries()).map(([handleId, entry]) => [asSpiceHandle(handleId), entry] as const), | ||
| }; |
There was a problem hiding this comment.
createSpiceHandleRegistry() exposes an internal __entries() hook (documented as internal) but returns a mutable snapshot of registry state. While it’s intended for backend cleanup, returning entries that include SpiceHandleEntry objects could allow accidental mutation of the entry objects (e.g. entry.kind = ...) in consumers of __entries().
Even if only internal backends call it today, this is an easy footgun and can cause confusing handle-kind mismatches later.
Suggestion
Make __entries() return deeply readonly data or defensive copies.
For example, clone entries:
__entries: () => Array.from(handles.entries()).map(([handleId, entry]) => [asSpiceHandle(handleId), { ...entry }] as const)
Or freeze them in dev if you prefer.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
@CharlieHelps I agree, that is a big concern. Let's move all of the necessary types into core such that tspice-core does not depend on tspice-backend-contract. tspice-core should have zero project-internal dependencies. |
|
Implemented — I moved the shared runtime types into What changed:
Validation run:
Commit:
|
|
Follow-up: I verified the commit is now on the actual PR head branch ( Current PR head:
So the refactor is now fully reflected in this PR branch. |
|
@CharlieHelps please review all relevant issue and PR context, then conduct a thorough code review. What “good” looks like
Workflow
Use this exact structure in the review summary: Blocking feedback
**Non-blocking feedback**
Rules:
|
There was a problem hiding this comment.
Blocking feedback
scripts/docs-api.mjscurrently compilesbackend-contractbeforecore, but this PR now makesbackend-contractimport@rybosome/tspice-core; on a clean build this causesTS2307module-resolution failures and is already failing multiple CI checks. This is merge-blocking becauseDocs build,check, andcheck-nativejobs are red for this reason. Please update docs API generation socoredeclarations are built beforebackend-contract(or switch to a dependency-awaretsc -bflow) so build order is deterministic.
**Non-blocking feedback**
SpiceHandleRegistrystill exposes optional__entries()through exported core types, which leaves an internal cleanup/debug hook reachable from the public API surface. That increases long-term API maintenance risk and blurs boundary intent. Consider splitting public vs internal registry types (or otherwise hiding__entriesfrom root exports) so the hook stays internal.
|
Finished a thorough review and posted a structured review with blocking/non-blocking feedback: #533 (review). GitHub blocked |
@CharlieHelps please address both of these issues |
|
Implemented — both items from your feedback are now addressed in
Validation run:
All passing locally in the PR branch. |
|
@CharlieHelps please review all relevant issue and PR context, then conduct a thorough code review. What “good” looks like
Workflow
Use this exact structure in the review summary: Blocking feedback
**Non-blocking feedback**
Rules:
|
There was a problem hiding this comment.
Blocking feedback
packages/backend-contractbuildandtypecheckare now order-dependent on prebuilt@rybosome/tspice-coredeclarations. Why this matters: from a clean checkout,pnpm -C packages/backend-contract buildandpnpm -C packages/backend-contract typecheckfail withTS2307untilpackages/corehas already been built, which makes package-level validation brittle and non-deterministic. Smallest fix: makebackend-contractscripts dependency-aware (prefertsc -bto honor project references, or prebuildpackages/coreinside those scripts).
**Non-blocking feedback**
AssertSpiceInt32Optionsis still declared/exported from@rybosome/tspice-backend-contracteven though runtimeSpiceInthelpers moved to@rybosome/tspice-core. Why this matters: duplicate type ownership can drift and leaves consumers with an ambiguous source for runtime-helper-adjacent types. Smallest fix: re-export/type-alias this from@rybosome/tspice-core(or remove the contract export if compatibility allows).createSpiceHandleRegistry().__entries()still returns liveSpiceHandleEntryobject references. Why this matters: internal/debug consumers can mutate returned objects and accidentally violate registry invariants. Smallest fix: return defensive read-only snapshots from__entries()(for example, clone eachentrybefore returning).
|
Completed final durable review (comment fallback): review. |
@CharlieHelps Please address all of these issues within this PR |
|
Implemented — I addressed all three items in
Validation run (all passing):
If you want, I can also run the remaining package typechecks from the PR checklist before merge. |
2 participants
Summary
packages/core/src/spice-runtime/*@rybosome/tspice-corepackages/backend-contractfocused on contract defs/constants (remove runtime helper exports)Validation
pnpm -C packages/backend-contract buildpnpm -C packages/backend-contract typecheckpnpm -C packages/backend-contract testpnpm -C packages/core buildpnpm -C packages/core typecheckpnpm -C packages/core testpnpm -C packages/backend-fake typecheckpnpm -C packages/backend-node typecheckpnpm -C packages/backend-wasm typecheckpnpm -C packages/tspice typecheckRefs #524