feat: op type class hierarchy (cycle 0009)#80
Merged
flyingrobots merged 14 commits intomainfrom Apr 7, 2026
Merged
Conversation
Pull PROTO_op-type-class-hierarchy (cool-ideas) and CC_op-types-typedef-to-class (bad-code) into cycle 0009. Hill: replace 8 typedef ops with frozen class hierarchy for runtime identity, constructor validation, and instanceof dispatch.
Base Op class (abstract) + 8 concrete subclasses: NodeAdd, NodeRemove, EdgeAdd, EdgeRemove, NodePropSet, EdgePropSet, PropSet, BlobValue. Each class: constructor validation, Object.freeze, instanceof dispatch. Edge types use options objects (max-params compliance). Shared validators in validate.js. Cycle 0009 ��� op-type-class-hierarchy
Factory functions in WarpTypesV2.js now delegate to Op class constructors. OpNormalizer.normalizeRawOp() and lowerCanonicalOp() already return class instances via the factory functions. Updated WarpTypesV2.test.js: real Dot instances replace plain objects, encoded dot strings replace object arrays for observedDots. 15 new factory-integration tests + 35 updated existing tests. Cycle 0009 — op-type-class-hierarchy
11 new tests verifying class-instance ops work through all three reducer paths (applyFast, applyWithReceipt, applyWithDiff) and OP_STRATEGIES dispatch. No source changes needed — the string-keyed strategy Map already dispatches correctly for class instances since they carry the same .type field. Cycle 0009 — op-type-class-hierarchy
Cycle 0009 — op-type-class-hierarchy
Witness: hill met — all 8 agent questions answered YES (Q9 deferred). All 3 human questions answered YES. Hard gates: noCoordination 7/7, 5504/5504 tests, lint clean. Retro: fast RED→GREEN, edge options objects caught by lint, existing tests had wrong Dot field names hidden by any-casts. New backlog: - PROTO_op-consumer-instanceof-migration (up-next, S) - PROTO_cbor-op-hydration (up-next, M) - CC_versionvector-constructor-no-validation (bad-code, S) Cycle 0009 — op-type-class-hierarchy
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR replaces plain-object op typedefs with a frozen Op class hierarchy (8 concrete subclasses), updates WarpTypesV2 factories to construct those classes, adds shared validation helpers, updates tests (~97 added/modified) and docs (design/retro/backlog), and adjusts CI/test scripts; CHANGELOG also notes auto-materialize-on-remove behavior for createPatch(). Changes
Sequence Diagram(s)sequenceDiagram
participant Factory as WarpTypesV2 (factory)
participant OpClass as Op subclasses
participant Normalizer as OpNormalizer
participant Reducer as JoinReducer
participant Store as Patch/Journal
Factory->>OpClass: construct specific Op subclass (e.g., NodeAdd, EdgeAdd)
OpClass-->>Factory: returns frozen instance (preserves `type`)
Factory->>Normalizer: provide raw or canonical op
Normalizer->>OpClass: normalizeRawOp / lowerCanonicalOp (map ↔ instances)
Reducer->>OpClass: applyOpV2 / applyFast with Op instances
Reducer->>Store: emit patch/receipt (patch.ops contain Op instances)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
test/unit/domain/types/WarpTypesV2.test.js (1)
442-447: Use distinctDotinstances per add-op fixture.This test currently reuses one
Dotfor bothNodeAddandEdgeAdd. Separate dots keep fixtures aligned with causal-identity expectations and avoid normalizing an invalid pattern.Proposed test tweak
- const dot = new Dot('w', 1); + const nodeDot = new Dot('w', 1); + const edgeDot = new Dot('w', 2); const ops = [ - createNodeAddV2('n1', dot), + createNodeAddV2('n1', nodeDot), createNodeRemoveV2('n1', []), - createEdgeAddV2('n1', 'n2', 'r', dot), + createEdgeAddV2('n1', 'n2', 'r', edgeDot), createEdgeRemoveV2('n1', 'n2', 'r', []), createPropSetV2('n1', 'k', 'v'), ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/types/WarpTypesV2.test.js` around lines 442 - 447, The test reuses one Dot instance for both NodeAdd and EdgeAdd which violates causal-identity expectations; update the ops fixture so createNodeAddV2 and createEdgeAddV2 each receive their own distinct Dot (e.g., two Dot(...) instances) instead of sharing the same dot variable; locate the ops array construction around the calls to createNodeAddV2, createNodeRemoveV2, createEdgeAddV2, and createEdgeRemoveV2 and replace the single shared Dot with separate Dot instances for the add operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/0009-op-type-class-hierarchy/op-type-class-hierarchy.md`:
- Around line 31-32: The design doc claims JoinReducer.OP_STRATEGIES dispatches
by instanceof but the implementation uses string-key lookup via op.type; update
the design doc text (references: JoinReducer.OP_STRATEGIES, JoinReducer.js line
showing OP_STRATEGIES.get(op.type)) to accurately state that strategy lookup is
performed by the `.type` string on operation instances, or explicitly document
that an instanceof-based dispatch approach is planned/deferred and not currently
used; ensure the doc references the `.type` property on class instances and
remove or rephrase any lines asserting instanceof dispatch.
In `@src/domain/types/ops/EdgeRemove.js`:
- Around line 36-40: The container-level assertArray call in the EdgeRemove
constructor is not enough—after assertArray(observedDots, 'EdgeRemove',
'observedDots') you must validate each element is a non-empty encoded dot string
before freezing; iterate observedDots and run the appropriate element validator
(e.g., an existing assertEncodedDot or a new small helper) on each entry
(include context 'EdgeRemove' and 'observedDots' and the element index in error
messages), then set this.observedDots = Object.freeze([...observedDots]).
In `@src/domain/types/ops/NodePropSet.js`:
- Around line 8-9: NodePropSet's constructor currently uses assertNonEmptyString
but does not reject NUL bytes for the identifiers; update the validation in the
NodePropSet constructor (the class/function named NodePropSet) to check both the
node and key parameters for banned NUL bytes (e.g., string.includes('\0')) and
throw/raise the same validation error when found; reuse or extend
assertNonEmptyString (or add a small helper) so both node and key are rejected
on construction if they contain '\0' (also apply the same check to the other
validation locations referenced in the comment around lines 33-35).
In `@src/domain/types/ops/NodeRemove.js`:
- Around line 29-31: The constructor for NodeRemove only asserts observedDots is
an array but doesn't validate elements; update the NodeRemove constructor to
iterate observedDots and validate each entry is a non-empty string (e.g., use or
add an assertString/assertNonEmpty helper) before freezing the array, and throw
a clear error mentioning 'NodeRemove' and 'observedDots' if any element is
invalid so downstream remove processing cannot receive bad entries.
In `@src/domain/types/ops/PropSet.js`:
- Around line 12-13: PropSet currently only calls assertNonEmptyString for the
key, which allows NUL bytes; update the PropSet constructor (or the helper it
uses) to reject any key containing '\0' by adding a NUL-byte check alongside the
existing assertNonEmptyString validation (e.g., throw if key.includes('\0'));
ensure the same change is applied for all PropSet key validations mentioned
(lines ~37-39) so every code path that sets PropSet.key enforces non-empty and
no NUL bytes, referencing the PropSet constructor and assertNonEmptyString
helper for where to add the check.
In `@test/unit/domain/types/ops/factory-integration.test.js`:
- Line 15: Remove the unused import by deleting the BlobValue import statement
(the "import BlobValue from ...BlobValue.js" line) from the test file; ensure
there are no remaining references to the BlobValue symbol in the file and run
the tests/linter to confirm the unused-import warning is resolved.
- Around line 120-129: The test's `canonical` variable is typed as a union so
TypeScript can't see `node`/`key` even after the runtime `toBeInstanceOf` check;
narrow the type before accessing properties by casting `canonical` to
`NodePropSet` (e.g., use a type assertion like `canonical as NodePropSet` or
assign `const nodeProp = canonical as NodePropSet`) and then assert
`nodeProp.node`, `nodeProp.key`, and `nodeProp.value`; keep using
`normalizeRawOp`, `NodePropSet`, and the `canonical` variable names to locate
the change.
---
Nitpick comments:
In `@test/unit/domain/types/WarpTypesV2.test.js`:
- Around line 442-447: The test reuses one Dot instance for both NodeAdd and
EdgeAdd which violates causal-identity expectations; update the ops fixture so
createNodeAddV2 and createEdgeAddV2 each receive their own distinct Dot (e.g.,
two Dot(...) instances) instead of sharing the same dot variable; locate the ops
array construction around the calls to createNodeAddV2, createNodeRemoveV2,
createEdgeAddV2, and createEdgeRemoveV2 and replace the single shared Dot with
separate Dot instances for the add operations.
🪄 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
Run ID: ba3cdbe1-183c-4e0d-b5dd-cac2ae425741
📒 Files selected for processing (25)
CHANGELOG.mddocs/design/0009-op-type-class-hierarchy/op-type-class-hierarchy.mddocs/design/0009-op-type-class-hierarchy/witness.mddocs/method/backlog/bad-code/CC_op-types-typedef-to-class.mddocs/method/backlog/bad-code/CC_versionvector-constructor-no-validation.mddocs/method/backlog/cool-ideas/PROTO_op-type-class-hierarchy.mddocs/method/backlog/up-next/PROTO_cbor-op-hydration.mddocs/method/backlog/up-next/PROTO_op-consumer-instanceof-migration.mddocs/method/retro/0009-op-type-class-hierarchy/retro.mdsrc/domain/types/WarpTypesV2.jssrc/domain/types/ops/BlobValue.jssrc/domain/types/ops/EdgeAdd.jssrc/domain/types/ops/EdgePropSet.jssrc/domain/types/ops/EdgeRemove.jssrc/domain/types/ops/NodeAdd.jssrc/domain/types/ops/NodePropSet.jssrc/domain/types/ops/NodeRemove.jssrc/domain/types/ops/Op.jssrc/domain/types/ops/PropSet.jssrc/domain/types/ops/validate.jstest/unit/domain/types/WarpTypesV2.test.jstest/unit/domain/types/ops/Op.test.jstest/unit/domain/types/ops/factory-integration.test.jstest/unit/domain/types/ops/reducer-integration.test.jstest/unit/domain/types/ops/validate.test.js
💤 Files with no reviewable changes (2)
- docs/method/backlog/bad-code/CC_op-types-typedef-to-class.md
- docs/method/backlog/cool-ideas/PROTO_op-type-class-hierarchy.md
Code review (12 issues, 0 remaining): C1/M2/m1/m2: Add assertNoReservedBytes to all op constructors — rejects NUL (\x00) and \x01 prefix in identifiers. Matches PatchBuilderV2._assertNoReservedBytes. PropSet exempted (legitimately carries \x01-prefixed nodes for edge prop wire encoding). M1: Design doc Status DESIGN → HILL MET. Added Delivered vs. Deferred table clarifying which scope items shipped. n1: Blank line between frontmatter fields in design doc. n2: Witness references test descriptions instead of line numbers. n3: Retro file count 9 → 10. n4: WarpTypesV2 import alias PropSetOpClass → PropSetClass. 15 new validation tests. 5519 total tests passing. Cycle 0009 — op-type-class-hierarchy
Use case: mirroring git history into a warp graph. HEAD snapshot is fast, but full history backfill is O(commits). Explores edge-based chronological ordering, checkpoint seeding, wormhole compression, and lazy materialization with continuation tokens. Cycle 0009 — op-type-class-hierarchy
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/types/WarpTypesV2.js`:
- Around line 160-162: The JSDoc `@returns` annotations still reference
plain-object typedefs (e.g., OpV2NodeAdd) while the factory functions like
createNodeAddV2, createNodeRemoveV2, createEdgeAddV2, createEdgeRemoveV2,
createPropSetV2, createNodePropSetV2, and createEdgePropSetV2 now return class
instances (NodeAddClass, NodeRemoveClass, EdgeAddClass, EdgeRemoveClass,
PropSetClass, NodePropSetClass, EdgePropSetClass); update each function's
`@returns` tag to reference the corresponding class type (e.g., `@returns`
{NodeAddClass}) OR change the typedefs to alias the classes (e.g., `@typedef`
{NodeAddClass} OpV2NodeAdd) so TypeScript sees them as compatible.
🪄 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
Run ID: 43fbdd8b-5655-418b-9c1d-4e366037d012
📒 Files selected for processing (15)
docs/design/0009-op-type-class-hierarchy/op-type-class-hierarchy.mddocs/design/0009-op-type-class-hierarchy/witness.mddocs/method/backlog/cool-ideas/PROTO_incremental-history-backfill.mddocs/method/retro/0009-op-type-class-hierarchy/retro.mdsrc/domain/types/WarpTypesV2.jssrc/domain/types/ops/BlobValue.jssrc/domain/types/ops/EdgeAdd.jssrc/domain/types/ops/EdgePropSet.jssrc/domain/types/ops/EdgeRemove.jssrc/domain/types/ops/NodeAdd.jssrc/domain/types/ops/NodePropSet.jssrc/domain/types/ops/NodeRemove.jssrc/domain/types/ops/validate.jstest/unit/domain/types/ops/Op.test.jstest/unit/domain/types/ops/validate.test.js
✅ Files skipped from review due to trivial changes (6)
- src/domain/types/ops/NodePropSet.js
- docs/method/backlog/cool-ideas/PROTO_incremental-history-backfill.md
- docs/method/retro/0009-op-type-class-hierarchy/retro.md
- test/unit/domain/types/ops/validate.test.js
- docs/design/0009-op-type-class-hierarchy/witness.md
- docs/design/0009-op-type-class-hierarchy/op-type-class-hierarchy.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/domain/types/ops/EdgeAdd.js
- src/domain/types/ops/EdgePropSet.js
- src/domain/types/ops/NodeRemove.js
- src/domain/types/ops/validate.js
- src/domain/types/ops/EdgeRemove.js
- test/unit/domain/types/ops/Op.test.js
Member
Author
|
Resolved the outstanding review feedback on this branch.
Local verification for the fix set:
|
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
69-79: Consider keeping one smoke check for the Node 22 Docker path.This block now validates the suite on the runner, but it no longer builds or runs
docker-compose.test.yml’stest-node22service. Sincenpm run test:node22is still a supported path, a lightweight advisory build/run for that image would keep Dockerfile/compose drift from going unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 69 - 79, The workflow removed the Docker-based smoke check for the Node 22 image; add a lightweight CI step after "Run unit + integration tests" that builds and briefly runs the test-node22 service from docker-compose.test.yml to detect Dockerfile/compose drift (use the compose file and service name test-node22); implement it as a short build-and-run smoke step (e.g., docker-compose -f docker-compose.test.yml build test-node22 then docker-compose -f docker-compose.test.yml up -d test-node22 and a quick health/exit check, then tear down) so the pipeline still validates the docker image used by npm run test:node22 without running the full test suite.package.json (1)
100-100: Reuse the existingtestentrypoint here.Line 100 re-embeds
vitest run test/unit test/integrationinstead of composing thetestscript on Lines 84-85, so the Node 22 suite now has another source of truth alongsidedocker-compose.test.yml:26-33. That makes the CI and Docker paths easier to drift apart.♻️ Proposed simplification
- "test:node22:ci": "GIT_STUNTS_DOCKER=1 sh -c 'npx vitest run test/unit test/integration && bats test/bats/'", + "test:node22:ci": "GIT_STUNTS_DOCKER=1 sh -c 'npm run test -- test/integration && bats test/bats/'",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 100, The "test:node22:ci" script duplicates the vitest invocation instead of reusing the existing "test" entrypoint; update the "test:node22:ci" script to call the central "test" script (e.g., GIT_STUNTS_DOCKER=1 sh -c 'npm run test' or equivalent) while preserving the original environment flag and any bats invocation so CI uses the single source of truth defined by the "test" script.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 69-79: The workflow removed the Docker-based smoke check for the
Node 22 image; add a lightweight CI step after "Run unit + integration tests"
that builds and briefly runs the test-node22 service from
docker-compose.test.yml to detect Dockerfile/compose drift (use the compose file
and service name test-node22); implement it as a short build-and-run smoke step
(e.g., docker-compose -f docker-compose.test.yml build test-node22 then
docker-compose -f docker-compose.test.yml up -d test-node22 and a quick
health/exit check, then tear down) so the pipeline still validates the docker
image used by npm run test:node22 without running the full test suite.
In `@package.json`:
- Line 100: The "test:node22:ci" script duplicates the vitest invocation instead
of reusing the existing "test" entrypoint; update the "test:node22:ci" script to
call the central "test" script (e.g., GIT_STUNTS_DOCKER=1 sh -c 'npm run test'
or equivalent) while preserving the original environment flag and any bats
invocation so CI uses the single source of truth defined by the "test" script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 504dbdcb-6bf4-402f-bd5d-f09fa147933d
📒 Files selected for processing (3)
.github/workflows/ci.ymldocs/method/backlog/bad-code/CC_versionvector-constructor-no-validation.mdpackage.json
✅ Files skipped from review due to trivial changes (1)
- docs/method/backlog/bad-code/CC_versionvector-constructor-no-validation.md
Release Preflight
If you tag this commit as |
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.
Summary
NodeAdd,NodeRemove,EdgeAdd,EdgeRemove,NodePropSet,EdgePropSet,PropSet,BlobValue) promoted to frozen classes with constructor validation andinstanceofdispatchOpclass (abstract) provides shared runtime identityWarpTypesV2.jsdelegate to class constructorsWhat's deferred
instanceofmigration (MessageSchemaDetector, presenter, TickReceipt) — backlogup-next/up-next/Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests