V18 Continuum slices 16-20#96
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR implements the V18 graph-model substrate layer (slices 17–20), introducing runtime-backed node, edge, and attachment record types; immutable graph operation classes and algebra; deterministic WarpState read surface for record materialization; and a projection service. Comprehensive tests and documentation accompany the implementation. ChangesV18 Graph-Model Substrate Convergence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Release Preflight
If you tag this commit as |
![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/graph/GraphOpAlgebra.test.ts (1)
7-9: ⚡ Quick winReplace literal op-name assertions with exported constants.
These string literals make the test brittle and violate the no-magic-strings rule; assert against the exported constants instead.
Proposed patch
import GraphAttachmentSetOp from '../../../../src/domain/graph/GraphAttachmentSetOp.ts'; import GraphEdgeRecordSetOp from '../../../../src/domain/graph/GraphEdgeRecordSetOp.ts'; import GraphNodeRecordSetOp from '../../../../src/domain/graph/GraphNodeRecordSetOp.ts'; +import { GRAPH_ATTACHMENT_SET_OP } from '../../../../src/domain/graph/GraphAttachmentSetOp.ts'; +import { GRAPH_EDGE_RECORD_SET_OP } from '../../../../src/domain/graph/GraphEdgeRecordSetOp.ts'; +import { GRAPH_NODE_RECORD_SET_OP } from '../../../../src/domain/graph/GraphNodeRecordSetOp.ts'; import GraphOpAlgebra from '../../../../src/domain/graph/GraphOpAlgebra.ts'; @@ expect(algebra.operations.map((operation) => operation.type)).toEqual([ - 'GraphNodeRecordSet', - 'GraphEdgeRecordSet', - 'GraphAttachmentSet', + GRAPH_NODE_RECORD_SET_OP, + GRAPH_EDGE_RECORD_SET_OP, + GRAPH_ATTACHMENT_SET_OP, ]);As per coding guidelines:
**/*.{ts,tsx}: Avoid magic strings and numbers; use named constants instead.Also applies to: 29-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/graph/GraphOpAlgebra.test.ts` around lines 7 - 9, The tests currently assert against literal op-name strings — update the assertions to use the exported op name constants from the corresponding modules instead: import and use the exported constant(s) from GraphAttachmentSetOp, GraphEdgeRecordSetOp, and GraphNodeRecordSetOp (e.g., the exported OP_NAME or similar constant defined in each file) wherever the test checks op names (including the other occurrences around lines 29-33) so the tests reference the module constants rather than hard-coded strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.ts`:
- Around line 308-321: index.ts has grown past the 500 LOC limit due to a large
flat export list (e.g., AttachmentKey, AttachmentRecord,
AttachmentSchemaVersion, EdgeId, EdgeRecord, EdgeTypeId, GraphAttachmentSetOp,
GraphEdgeRecordSetOp, GraphNodeRecordSetOp, GraphOpAlgebra,
GraphOpAlgebraProjection, NodeId, NodeRecord, NodeTypeId). Split the surface by
moving related export groups into narrower barrel modules (for example create a
new graph-types.ts or attachments.ts that export Attachment*, Edge*, Node* and
Graph* symbols), update index.ts to re-export those barrels (export * from
'./graph-types') instead of listing every symbol, and remove the expanded list
from index.ts so it falls back under the 500 LOC policy; ensure all original
symbol names (AttachmentKey, EdgeRecord, GraphOpAlgebraProjection, NodeTypeId,
etc.) are re-exported from the new barrels so existing imports keep working.
In `@src/domain/graph/NodeId.ts`:
- Around line 21-22: The equals method in NodeId (equals(other: NodeId):
boolean) must be made total by returning false for null/undefined or non-NodeId
inputs; update the function to first check that other is an instance of NodeId
(or at least truthy and has a compatible value shape) and return false if not,
then compare this.value === other.value; mirror the same guard pattern used by
the sibling identifier/value classes to keep behavior consistent.
In `@src/domain/services/state/WarpState.ts`:
- Around line 100-107: getEdgeRecord currently calls edgeRecords() each time
which rebuilds/sorts the full projection and causes N^2 behavior during
attachmentRecords materialization; fix by creating a single lookup map from
EdgeId to EdgeRecord (e.g. build a Map once from this.edgeRecords()) and use
that map instead of repeatedly calling edgeRecords() — either (a) change
attachmentRecords to build the map once and call a new helper/getEdgeRecord
overload that accepts the map, or (b) add a short-lived cachedEdgeMap property
computed before materialization and have getEdgeRecord consult it; update
references to getEdgeRecord/attachmentRecords/WarpState to use the map-based
lookup so the full projection is not rebuilt per edge property.
In `@test/unit/domain/graph/AttachmentRecord.test.ts`:
- Around line 33-65: Add tests that exercise the constructor guard branches of
AttachmentRecord by asserting it throws on invalid inputs: create cases that
pass a null/undefined owner, a non-AttachmentKey key, missing value, and an
invalid schema version to the AttachmentRecord constructor and expect errors;
reference the AttachmentRecord constructor (and helpers
NodeRecord.fromLegacyNodeId, EdgeRecord.fromLegacyEdge, AttachmentKey,
AttachmentSchemaVersion) to locate where to instantiate invalid payloads and use
expect(...).toThrow() (or the async equivalent) for each failure mode so the
negative paths are covered.
In `@test/unit/domain/graph/EdgeRecord.test.ts`:
- Around line 33-49: Add negative-path unit tests that exercise EdgeRecord's
validation branches: call EdgeRecord.fromLegacyEdge and the EdgeRecord
constructor with missing/invalid inputs to trigger requireFields, requireEdgeId,
requireNodeId, and requireEdgeTypeId, asserting they throw the expected errors;
specifically include cases like missing "from"/"to"/"label", invalid edge id
strings passed to requireEdgeId, and invalid node/type ids for
requireNodeId/requireEdgeTypeId, and verify the factory still returns a frozen
instance on success (EdgeRecord.fromLegacyEdge, EdgeRecord constructor,
requireFields, requireEdgeId, requireNodeId, requireEdgeTypeId).
In `@test/unit/domain/graph/GraphOpAlgebra.test.ts`:
- Around line 41-58: Add tests that validate boundary cases for the touched
constructors: assert that GraphNodeRecordSetOp, GraphEdgeRecordSetOp, and
GraphAttachmentSetOp throw when passed null or undefined as their
record/envelope, and assert that GraphOpAlgebra throws when constructed with
null/undefined or with an operations field that is not an array (e.g., a string
or object). Reference the constructors GraphNodeRecordSetOp,
GraphEdgeRecordSetOp, GraphAttachmentSetOp, and GraphOpAlgebra in the new test
cases and use the same toThrow checks (matching error messages like
/NodeRecord/, /EdgeRecord/, /AttachmentRecord/, /graph operation/) to ensure the
null/undefined and non-array paths are covered.
In `@test/unit/domain/graph/NodeRecord.test.ts`:
- Line 5: Replace the hardcoded default type string in the NodeRecord tests with
the exported constant from NodeTypeId: import and use the exported default-type
constant from NodeTypeId (instead of the literal) in the assertions in
NodeRecord.test.ts where the default type is currently asserted (both
occurrences), so the tests reference NodeTypeId's exported default constant
rather than a magic string.
---
Nitpick comments:
In `@test/unit/domain/graph/GraphOpAlgebra.test.ts`:
- Around line 7-9: The tests currently assert against literal op-name strings —
update the assertions to use the exported op name constants from the
corresponding modules instead: import and use the exported constant(s) from
GraphAttachmentSetOp, GraphEdgeRecordSetOp, and GraphNodeRecordSetOp (e.g., the
exported OP_NAME or similar constant defined in each file) wherever the test
checks op names (including the other occurrences around lines 29-33) so the
tests reference the module constants rather than hard-coded strings.
🪄 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: 362dc157-604c-4b1a-98c0-4a8fd9fb2989
📒 Files selected for processing (39)
CHANGELOG.mddocs/BEARING.mddocs/design/0164-v18-post-15-graph-model-runway/v18-post-15-graph-model-runway.mddocs/design/0165-v18-node-record-identity/v18-node-record-identity.mddocs/design/0166-v18-edge-record-identity/v18-edge-record-identity.mddocs/design/0167-v18-attachment-plane-substrate/v18-attachment-plane-substrate.mddocs/design/0168-v18-graph-op-algebra-convergence/v18-graph-op-algebra-convergence.mddocs/method/backlog/WORKLOADS.mddocs/method/backlog/v18.0.0/PROTO_attachment-plane-substrate.mddocs/method/backlog/v18.0.0/PROTO_echo-shaped-edge-records.mddocs/method/backlog/v18.0.0/PROTO_echo-shaped-node-records.mddocs/method/backlog/v18.0.0/PROTO_graph-op-algebra-convergence.mddocs/method/backlog/v18.0.0/README.mdindex.tssrc/domain/graph/AttachmentKey.tssrc/domain/graph/AttachmentRecord.tssrc/domain/graph/AttachmentSchemaVersion.tssrc/domain/graph/EdgeId.tssrc/domain/graph/EdgeRecord.tssrc/domain/graph/EdgeTypeId.tssrc/domain/graph/GraphAttachmentSetOp.tssrc/domain/graph/GraphEdgeRecordSetOp.tssrc/domain/graph/GraphNodeRecordSetOp.tssrc/domain/graph/GraphOpAlgebra.tssrc/domain/graph/GraphOperation.tssrc/domain/graph/NodeId.tssrc/domain/graph/NodeRecord.tssrc/domain/graph/NodeTypeId.tssrc/domain/services/GraphOpAlgebraProjection.tssrc/domain/services/state/WarpState.tstest/unit/domain/graph/AttachmentRecord.test.tstest/unit/domain/graph/EdgeRecord.test.tstest/unit/domain/graph/GraphOpAlgebra.test.tstest/unit/domain/graph/NodeRecord.test.tstest/unit/domain/index.exports.test.tstest/unit/domain/services/GraphOpAlgebraProjection.test.tstest/unit/domain/services/state/WarpState.attachmentRecords.test.tstest/unit/domain/services/state/WarpState.edgeRecords.test.tstest/unit/domain/services/state/WarpState.nodeRecords.test.ts
💤 Files with no reviewable changes (4)
- docs/method/backlog/v18.0.0/PROTO_echo-shaped-node-records.md
- docs/method/backlog/v18.0.0/PROTO_echo-shaped-edge-records.md
- docs/method/backlog/v18.0.0/PROTO_graph-op-algebra-convergence.md
- docs/method/backlog/v18.0.0/PROTO_attachment-plane-substrate.md
Release Preflight
If you tag this commit as |
This was referenced May 23, 2026
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
WarpStaterecord projections.Design Docs
docs/design/0164-v18-post-15-graph-model-runway/v18-post-15-graph-model-runway.mddocs/design/0165-v18-node-record-identity/v18-node-record-identity.mddocs/design/0166-v18-edge-record-identity/v18-edge-record-identity.mddocs/design/0167-v18-attachment-plane-substrate/v18-attachment-plane-substrate.mddocs/design/0168-v18-graph-op-algebra-convergence/v18-graph-op-algebra-convergence.mdVerification
npx vitest run test/unit/domain/graph/GraphOpAlgebra.test.ts test/unit/domain/services/GraphOpAlgebraProjection.test.ts test/unit/domain/index.exports.test.ts --reporter=verbosenpm run typechecknpm run lintnpm run lint:sludgenpm run lint:quarantine-graduatenpm run test:localgit diff --check origin/main...HEADSummary by CodeRabbit
New Features
Bug Fixes
Documentation