Complete V18-GP4 streamed optic slices#643
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 24 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThis pull request implements neighborhood and traversal optics for v18 holographic graph slicing, backed by a complete checkpoint basis infrastructure. It adds one-hop and multi-hop graph navigation with deterministic witness output, bounded shard-resident reads, and a new CLI ChangesHolographic Optic Reads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Member
Author
|
@codex Self-review found one actionable issue before merge.
|
Release Preflight
If you tag this commit as |
Member
Author
|
@codex Self-review issue resolved.
|
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/services/optic/CheckpointBasisManifest.ts (1)
1-545: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftThis source file exceeds the size cap and mixes too many peer concepts.
At 545 LOC, this file is over the source-file limit and combines multiple domain value objects plus manifest orchestration/validators in one place.
As per coding guidelines,
**/*.{ts,tsx,js,mjs}source files must stay within 500 LOC, and**/*.{ts,tsx}should use one file per class/type/object.🤖 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 `@src/domain/services/optic/CheckpointBasisManifest.ts` around lines 1 - 545, The file is too large and mixes multiple domain VOs and validation/orchestration; split it into smaller modules: extract CheckpointBasisManifest (with constructor, rootMaps, validateManifestState call) into its own file, extract each value object class (CheckpointBasisShardRootMap, CheckpointBasisShardGeometry, CheckpointBasisChunking, CheckpointBasisCompleteness, CheckpointBasisSupportPosture) into separate files (one per class), and move related validators/helpers (validateManifestOptions, manifestRootMaps, nullableManifestRefs, validateShardGeometryObjects, validateReadingIdentitySeparation, initialByteIdentityValues, add*IdentityValues, throwManifestError, freezeStringList, copy* helpers, compareMapEntries, validation primitives like validateNonEmptyString/validatePositiveInteger/validateNonNegativeInteger/validateSupportedSchema/validateRootFamily/validateCompletenessKind/validatePostureKind/validateFrontier/validateAppliedVersionVector/validateRoots/validateSupportPosture/validateCompleteness/validateShardGeometry/validatePostureShape/validationPostureOrDefault/verificationPostureOrDefault) into a validators/utilities module; update imports/exports so CheckpointBasisManifest imports the VO classes and validator functions by name (e.g., CheckpointBasisShardRootMap, CheckpointBasisShardGeometry, CheckpointBasisChunking, CheckpointBasisCompleteness, CheckpointBasisSupportPosture, validateManifestOptions, manifestRootMaps, nullableManifestRefs, validateManifestState, throwManifestError), preserve exported CHECKPOINT_BASIS_MANIFEST_SCHEMA and types, and run tests to ensure no API changes.Source: Coding guidelines
🧹 Nitpick comments (1)
test/conformance/fixtures/V17PublicOpticReadPath.ts (1)
24-34: ⚡ Quick winExtract reflected method names into constants.
Line 26 through Line 33 and Line 60 add more string-literal reflective method names; centralizing them as constants lowers typo risk in this dynamic dispatch path.
As per coding guidelines, "Use named constants instead of magic strings or numbers in TypeScript".Proposed refactor
+const WORLDLINE_OPTIC_METHODS = Object.freeze({ + optic: 'optic', + neighborhood: 'neighborhood', + traversal: 'traversal', + read: 'read', +} as const); + async readNeighborhood(nodeId: string, options: object): Promise<object> { - const optic = this.invokeObject(this.worldline, 'optic'); - const neighborhoodScope = this.invokeObject(optic, 'neighborhood', [nodeId]); - return await this.invokePromiseObject(neighborhoodScope, 'read', [options]); + const optic = this.invokeObject(this.worldline, WORLDLINE_OPTIC_METHODS.optic); + const neighborhoodScope = this.invokeObject(optic, WORLDLINE_OPTIC_METHODS.neighborhood, [nodeId]); + return await this.invokePromiseObject(neighborhoodScope, WORLDLINE_OPTIC_METHODS.read, [options]); } async readTraversal(startNodeId: string, options: object): Promise<object> { - const optic = this.invokeObject(this.worldline, 'optic'); - const traversalScope = this.invokeObject(optic, 'traversal', [startNodeId]); - return await this.invokePromiseObject(traversalScope, 'read', [options]); + const optic = this.invokeObject(this.worldline, WORLDLINE_OPTIC_METHODS.optic); + const traversalScope = this.invokeObject(optic, WORLDLINE_OPTIC_METHODS.traversal, [startNodeId]); + return await this.invokePromiseObject(traversalScope, WORLDLINE_OPTIC_METHODS.read, [options]); }Also applies to: 60-60
🤖 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/conformance/fixtures/V17PublicOpticReadPath.ts` around lines 24 - 34, The code in readNeighborhood and readTraversal uses repeated string-literal reflective method names ("optic", "neighborhood", "traversal", "read") which should be centralized into named constants to reduce typos and improve maintainability; update V17PublicOpticReadPath.ts by defining constants (e.g., OPTIC = 'optic', NEIGHBORHOOD = 'neighborhood', TRAVERSAL = 'traversal', READ = 'read') and replace the inline literals in the calls to invokeObject(this.worldline, 'optic'), invokeObject(optic, 'neighborhood', ...), invokeObject(optic, 'traversal', ...) and invokePromiseObject(..., 'read', ...) (also used at line ~60) to use these constants.Source: Coding guidelines
🤖 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 `@src/domain/services/optic/CheckpointBasisFact.ts`:
- Around line 12-423: File groups too many peer concepts in one module: split
the transports, base class, concrete fact classes, and validators/helpers into
focused files. Create e.g. CheckpointBasisFact.types.ts
(CheckpointBasisFactTransport, CheckpointFactEventTransport and related
transport unions), CheckpointBasisFact.base.ts (abstract CheckpointBasisFact and
shard-family type), one file per concrete class (CheckpointNodeLivenessFact.ts,
CheckpointNodePropertyFact.ts, CheckpointAdjacencyFact.ts,
CheckpointEdgeFact.ts, CheckpointProvenanceFact.ts,
CheckpointContentAnchorFact.ts) and a helpers file (eventTransport,
eventSortKey, validateDirection, validatePropValue, validateEventId,
validateText, validatePositiveInteger, throwFactError, computeShardKey if
referenced elsewhere); move the constructors/logic into those class files and
export/import the types/helpers as needed, update all import sites to the new
module layout, ensure each file exports only its responsibility, run the
TypeScript compiler and tests to fix any import/export name issues and adjust
exports to preserve the public API (names like CheckpointNodeLivenessFact,
eventTransport, validateEventId, etc.).
In `@src/domain/services/optic/CheckpointBasisManifest.ts`:
- Around line 72-73: The frontier and appliedVersionVector Map fields on
CheckpointBasisManifest remain mutable after construction; make defensive
immutable copies in the constructor (e.g., create new Map(...) instances from
the inputs) and store them as private readonly fields, then expose them only as
ReadonlyMap via getters (or return new Map copies on access) so external callers
cannot mutate internal Map state; apply the same defensive-copying/readonly
exposure pattern for other assignments of frontier and appliedVersionVector in
this class (the other constructor/assignment sites referenced).
In `@src/domain/services/optic/CheckpointPatchFactStream.ts`:
- Around line 133-145: Move creation of the EventId into the try block inside
lowerOperation so EventId construction failures are handled by the stream's
obstruction contract; specifically, inside lowerOperation wrap both new
EventId(...) and the normalizeRawOp/raw op handling in the same try. In the
catch, preserve explicit "unsupported-operation" errors instead of converting
them to a generic malformed-patch: if the caught error is an Error whose message
or name equals "unsupported-operation" (or a known UnsupportedOperationError
class), rethrow it unchanged so callers see the original obstruction reason;
otherwise extract the error message and return malformedOperationFacts(cause) as
before. Ensure callers of normalizeRawOp and factsForOperation still surface
"unsupported-operation" when thrown.
In `@src/domain/services/optic/CheckpointShardFactReader.ts`:
- Around line 154-170: The neighborhoodShardOidMap currently adds all liveness
shards up front (via addShardRoots) which couples neighborhood reads to global
liveness health; change it to a two-phase approach: first only add adjacency and
edge fact roots needed for the requested node (use computeShardKey and
addOptionalShardRoot to add fwd_/rev_ and labels.cbor roots and return early if
none), then inspect the adjacency results to collect the specific node IDs
(source + neighbor IDs) and only then add liveness shard OIDs for those specific
shard keys (use addOptionalShardRoot or an equivalent that adds individual
liveness roots rather than addShardRoots). Apply the same two-phase pattern to
the analogous logic at lines 262-280 so neighborhood reads only load liveness
shards for nodes actually present in the adjacency set.
In `@src/domain/services/optic/CheckpointTailReadIdentityBuilder.ts`:
- Around line 84-89: labelsAspect currently produces non-unique encodings (e.g.,
[] vs ['all-labels'] and labels containing commas), so change labelsAspect to a
collision-safe, deterministic encoder: sort the labels, then encode each label
with an unambiguous escaping or length-prefix (for example prefix each label
with its byte/char length or percent-encode/escape delimiters) and join with a
delimiter that cannot appear unescaped; ensure the empty-array case is distinct
(e.g., explicit marker like "EMPTY" or an empty-length prefix sequence). Update
the function labelsAspect to implement this injective encoding so different
label arrays always produce different strings.
In `@src/domain/services/optic/CheckpointTailTraversalReader.ts`:
- Around line 119-142: The bug is that maybeOpenForNodeLimit re-queues the same
TraversalOpticFrontierEntry (current) when maxNodes is reached, causing resumed
reads to reprocess the same neighborhood slice and livelock; instead, when the
node limit is hit you should advance the frontier past the neighbor that
triggered the limit (do not re-add the identical current). Modify
maybeOpenForNodeLimit so it pushes a new/updated TraversalOpticFrontierEntry
representing continuation after the current neighbor (e.g., with the
neighborhood cursor/position advanced) into state.frontier (or omit re-queuing
current and let maybeOpenForNeighborhoodCursor handle opening the next cursor),
then call openTraversalResult(state); reference functions/types:
maybeOpenForNodeLimit, openTraversalResult, TraversalOpticFrontierEntry,
state.frontier, current, neighborhood.cursor. Ensure the re-queued entry
advances the cursor/index so resumed reads do not revisit the same slice.
In `@src/domain/services/optic/TraversalOptic.ts`:
- Around line 25-31: Add constructor invariants to TraversalOptic: validate that
options.startNodeId is a non-empty string and that options.locator is a valid
CheckpointTailWitnessLocator (e.g., instanceof CheckpointTailWitnessLocator or
has required properties), and throw a descriptive TypeError if validation fails;
perform these checks before assigning to this._startNodeId / this._locator and
before Object.freeze(this) to ensure the object cannot be constructed with
invalid inputs.
In `@test/unit/domain/services/optic/CheckpointPatchFactStream.test.ts`:
- Around line 79-126: Add a new test in CheckpointPatchFactStream.test.ts that
mirrors the existing malformed/coverage cases but injects an operation that
triggers the unsupported-operation path: create a TestPatchFactStreamSource,
setChain with a patchEntry whose ops include an operation type the stream does
not support (e.g., a custom/unknown Node* subclass or a clearly unsupported op
instance), instantiate CheckpointPatchFactStream with that source, call
collectFacts(stream.stream({...})) with the same
previousCheckpoint/targetFrontier setup used in the other tests, and assert the
rejection matches code 'E_CHECKPOINT_PATCH_FACT_STREAM' and context { field:
'patch.ops', reason: 'unsupported-operation' }; reuse helpers like patchEntry,
TestPatchFactStreamSource, collectFacts and the stream() call to keep the test
consistent with the existing ones.
---
Outside diff comments:
In `@src/domain/services/optic/CheckpointBasisManifest.ts`:
- Around line 1-545: The file is too large and mixes multiple domain VOs and
validation/orchestration; split it into smaller modules: extract
CheckpointBasisManifest (with constructor, rootMaps, validateManifestState call)
into its own file, extract each value object class (CheckpointBasisShardRootMap,
CheckpointBasisShardGeometry, CheckpointBasisChunking,
CheckpointBasisCompleteness, CheckpointBasisSupportPosture) into separate files
(one per class), and move related validators/helpers (validateManifestOptions,
manifestRootMaps, nullableManifestRefs, validateShardGeometryObjects,
validateReadingIdentitySeparation, initialByteIdentityValues,
add*IdentityValues, throwManifestError, freezeStringList, copy* helpers,
compareMapEntries, validation primitives like
validateNonEmptyString/validatePositiveInteger/validateNonNegativeInteger/validateSupportedSchema/validateRootFamily/validateCompletenessKind/validatePostureKind/validateFrontier/validateAppliedVersionVector/validateRoots/validateSupportPosture/validateCompleteness/validateShardGeometry/validatePostureShape/validationPostureOrDefault/verificationPostureOrDefault)
into a validators/utilities module; update imports/exports so
CheckpointBasisManifest imports the VO classes and validator functions by name
(e.g., CheckpointBasisShardRootMap, CheckpointBasisShardGeometry,
CheckpointBasisChunking, CheckpointBasisCompleteness,
CheckpointBasisSupportPosture, validateManifestOptions, manifestRootMaps,
nullableManifestRefs, validateManifestState, throwManifestError), preserve
exported CHECKPOINT_BASIS_MANIFEST_SCHEMA and types, and run tests to ensure no
API changes.
---
Nitpick comments:
In `@test/conformance/fixtures/V17PublicOpticReadPath.ts`:
- Around line 24-34: The code in readNeighborhood and readTraversal uses
repeated string-literal reflective method names ("optic", "neighborhood",
"traversal", "read") which should be centralized into named constants to reduce
typos and improve maintainability; update V17PublicOpticReadPath.ts by defining
constants (e.g., OPTIC = 'optic', NEIGHBORHOOD = 'neighborhood', TRAVERSAL =
'traversal', READ = 'read') and replace the inline literals in the calls to
invokeObject(this.worldline, 'optic'), invokeObject(optic, 'neighborhood', ...),
invokeObject(optic, 'traversal', ...) and invokePromiseObject(..., 'read', ...)
(also used at line ~60) to use these constants.
🪄 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: 5db830e0-9247-4774-a30d-fb1c120504aa
📒 Files selected for processing (33)
AGENTS.mdbin/cli/commands/optic.tsbin/cli/commands/registry.tsbin/cli/infrastructure.tsbin/cli/schemas.tssrc/domain/services/optic/CheckpointBasisFact.tssrc/domain/services/optic/CheckpointBasisManifest.tssrc/domain/services/optic/CheckpointPatchFactStream.tssrc/domain/services/optic/CheckpointShardFactReader.tssrc/domain/services/optic/CheckpointTailBasisLoader.tssrc/domain/services/optic/CheckpointTailFactReducer.tssrc/domain/services/optic/CheckpointTailReadFailure.tssrc/domain/services/optic/CheckpointTailReadIdentityBuilder.tssrc/domain/services/optic/CheckpointTailTraversalReader.tssrc/domain/services/optic/CheckpointTailWitnessLocator.tssrc/domain/services/optic/NeighborhoodOptic.tssrc/domain/services/optic/NeighborhoodOpticReadResult.tssrc/domain/services/optic/NodeOptic.tssrc/domain/services/optic/OpticReadFailureCause.tssrc/domain/services/optic/OpticReadTarget.tssrc/domain/services/optic/StreamingCheckpointBasisBuilder.tssrc/domain/services/optic/TraversalOptic.tssrc/domain/services/optic/TraversalOpticReadResult.tssrc/domain/services/optic/WorldlineOptic.tstest/conformance/fixtures/V17OpticFailureExpectations.tstest/conformance/fixtures/V17PublicOpticReadPath.tstest/conformance/v18NeighborhoodOpticReadBasis.test.tstest/conformance/v18TraversalOpticReadBasis.test.tstest/unit/cli/optic.test.tstest/unit/domain/services/optic/CheckpointBasisManifest.test.tstest/unit/domain/services/optic/CheckpointPatchFactStream.test.tstest/unit/domain/services/optic/CheckpointShardFactReader.manifest.test.tstest/unit/domain/services/optic/StreamingCheckpointBasisBuilder.test.ts
Member
Author
|
@codex Code Lawyer activity summary for PR #643.
Verification run locally after fixes:
Current status: pushed to |
Release Preflight
If you tag this commit as |
Member
Author
|
@codex Code Lawyer activity summary after CodeRabbit review threads.
Local validation after fixes:
All seven review threads have been resolved via GraphQL after the fix commits were pushed. |
Release Preflight
If you tag this commit as |
This was referenced Jun 6, 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
optic witnessCLI playback with deterministic basis and read-identity evidenceIssues
Verification
npm run lintnpm run typecheck:srcnpm run typecheck:testnpx vitest run test/conformance/v18NeighborhoodOpticReadBasis.test.ts test/conformance/v18TraversalOpticReadBasis.test.ts test/unit/cli/optic.test.ts test/unit/cli/parseArgs.test.ts test/unit/domain/services/optic/CheckpointBasisManifest.test.ts test/unit/domain/services/optic/StreamingCheckpointBasisBuilder.test.ts test/unit/domain/services/optic/CheckpointPatchFactStream.test.tsnpm run test:localNotes
Summary by CodeRabbit
Release Notes
New Features
optic witnessCLI command to query graph data with optional property filtering.Documentation