Close coordinate-backed Optics path#111
Open
flyingrobots wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR lands the v18 Optics public API closeout, enabling coordinate-backed coherent optic reads through a stable causal position without materializing the full graph. New Changesv18 Optics Public API Closeout: Coordinate-Backed Coherent Reads
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
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/CheckpointTailBasisLoader.ts (1)
59-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat empty checkpoint SHA as missing at the boundary.
_readCheckpointSha()currently only rejectsnull. An empty SHA should also map tomissing-checkpointto avoid downstream opaque persistence failures.Suggested patch
private async _readCheckpointSha(): Promise<string> { const checkpointSha = await this._source._readCheckpointSha(); - if (checkpointSha === null) { + if (checkpointSha === null || checkpointSha.length === 0) { throwNoBoundedBasis(this._source.graphName, 'missing-checkpoint'); } return checkpointSha; }🤖 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/CheckpointTailBasisLoader.ts` around lines 59 - 64, In _readCheckpointSha(), treat an empty string the same as null: after calling this._source._readCheckpointSha(), check if checkpointSha is null or checkpointSha === '' and if so call throwNoBoundedBasis(this._source.graphName, 'missing-checkpoint'); otherwise return the SHA; update the _readCheckpointSha method (referencing this._source._readCheckpointSha, checkpointSha, this._source.graphName, and throwNoBoundedBasis) so empty strings are mapped to the missing-checkpoint boundary error.
🤖 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/CoordinateCheckpointTailOpticSource.ts`:
- Around line 27-38: The constructor currently trusts options.frontier and
fields on options.source and can throw raw TypeErrors; add defensive validation
at the start of the CoordinateCheckpointTailOpticSource constructor to assert
options.source exists and has required identity fields (graphName, _persistence,
_codec, _blobStorage, _commitMessageCodec) using assertNonEmpty where
appropriate, validate options.checkpointSha (already present) and validate
options.frontier is non-null and shape-compatible before calling copyFrontier;
replace raw errors by throwing a WarpError (with a clear message) if validation
fails, and wrap the copyFrontier call in a try/catch that converts unexpected
exceptions into a WarpError so the constructor always enforces invariants rather
than leaking TypeErrors.
In `@src/domain/services/Worldline.ts`:
- Around line 136-139: Update the thrown QueryError in the unsupported-selector
branch to use a clear, accurate message instead of "v17 foundation optics
support live worldlines only"; locate the throw new QueryError(...) with code
'E_OPTIC_NO_BOUNDED_BASIS' (inside the unsupported-selector branch that
references this._source.constructor.name) and change the error text to describe
the actual unsupported selector case (e.g., indicate that the selector type is
unsupported or must be a coordinate selector), keeping the same error code and
context payload.
In `@src/domain/types/CoordinateSelector.ts`:
- Around line 81-86: The validateCheckpointSha function currently accepts
strings that are only whitespace; update its validation in validateCheckpointSha
to first ensure checkpointSha is a string, then trim() it and reject if the
trimmed length is 0 (throw the same QueryError message), returning the trimmed
value (or null when input is null/undefined) so whitespace-only inputs are
treated as invalid and the selector invariant is preserved.
In `@src/domain/WarpWorldlineCoordinate.ts`:
- Around line 25-33: The constructor of WarpWorldlineCoordinate must validate
the frontier and createWorldline inputs up front: add explicit runtime checks in
the constructor for options.frontier to be a non-null object with an entries
array (or whatever shape freezeFrontier expects) and for options.createWorldline
to be either undefined or a function before calling freezeFrontier or assigning
to this._createWorldline; if validation fails throw a domain error (e.g.,
TypeError or a custom error) with a clear message. Locate the constructor in
class WarpWorldlineCoordinate and add these checks prior to calling
freezeFrontier(options.frontier) and before setting this._createWorldline so
invalid inputs fail fast and with a domain-style error. Ensure the checks mirror
the same validation you will add in the other constructor uses referenced around
the class (lines noted in the review) so all construction paths enforce the
invariants.
In `@src/domain/WarpWorldlineOpticBasis.ts`:
- Around line 22-24: The assertNonEmpty validator currently allows strings that
are only whitespace; update the function assertNonEmpty(value: string, field:
string) used by WarpWorldlineOpticBasis to treat whitespace-only values as empty
by checking value.trim().length === 0 (or using a regex) in addition to the
existing typeof check, and continue to throw WarpError(field + "...") when
validation fails so the constructor enforces the non-whitespace invariant for
identity fields.
---
Outside diff comments:
In `@src/domain/services/optic/CheckpointTailBasisLoader.ts`:
- Around line 59-64: In _readCheckpointSha(), treat an empty string the same as
null: after calling this._source._readCheckpointSha(), check if checkpointSha is
null or checkpointSha === '' and if so call
throwNoBoundedBasis(this._source.graphName, 'missing-checkpoint'); otherwise
return the SHA; update the _readCheckpointSha method (referencing
this._source._readCheckpointSha, checkpointSha, this._source.graphName, and
throwNoBoundedBasis) so empty strings are mapped to the missing-checkpoint
boundary error.
🪄 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: b89ceb76-8bbb-4362-bb9b-ab2e0d793a17
📒 Files selected for processing (31)
CHANGELOG.mdREADME.mddocs/API_REFERENCE.mddocs/BEARING.mddocs/READINGS_AND_OPTICS.mddocs/design/0265-v18-optics-public-api-closeout/v18-optics-public-api-closeout.mddocs/method/backlog/README.mddocs/method/backlog/v18.0.0/API_optics-public-api-closeout.mddocs/method/backlog/v18.0.0/README.mddocs/method/backlog/v18.0.0/RELEASE_v18-public-release-blockers.mddocs/migrations/v18.0.0.mddocs/releases/v18.0.0/README.mdindex.tssrc/domain/RuntimeHost.tssrc/domain/WarpWorldline.tssrc/domain/WarpWorldlineCoordinate.tssrc/domain/WarpWorldlineOpticBasis.tssrc/domain/capabilities/QueryCapability.tssrc/domain/services/Worldline.tssrc/domain/services/controllers/CheckpointController.tssrc/domain/services/controllers/QueryController.tssrc/domain/services/optic/CheckpointTailBasisLoader.tssrc/domain/services/optic/CheckpointTailOpticSource.tssrc/domain/services/optic/CoordinateCheckpointTailOpticSource.tssrc/domain/types/CoordinateSelector.tssrc/domain/types/WorldlineSelector.tssrc/domain/warp/RuntimeHostProduct.tstest/conformance/v18CoordinateOpticPublicPath.test.tstest/type-check/consumer.tstest/unit/index.exports.test.tstest/unit/scripts/v18-worldline-api-doc-guard.test.ts
Comment on lines
+27
to
+38
| constructor(options: CoordinateCheckpointTailOpticSourceOptions) { | ||
| super(); | ||
| this.graphName = options.source.graphName; | ||
| this._persistence = options.source._persistence; | ||
| this._codec = options.source._codec; | ||
| this._blobStorage = options.source._blobStorage; | ||
| this._commitMessageCodec = options.source._commitMessageCodec; | ||
| this._source = options.source; | ||
| assertNonEmpty(options.checkpointSha, 'checkpointSha'); | ||
| this._checkpointSha = options.checkpointSha; | ||
| this._frontier = copyFrontier(options.frontier); | ||
| Object.freeze(this); |
There was a problem hiding this comment.
Add defensive constructor validation for frontier and identity fields.
With malformed runtime input, this path can currently throw raw TypeErrors instead of a WarpError at the boundary.
Suggested fix
constructor(options: CoordinateCheckpointTailOpticSourceOptions) {
super();
@@
+ if (!(options.frontier instanceof Map)) {
+ throw new WarpError(
+ 'Coordinate checkpoint-tail optic source requires a frontier Map',
+ 'E_COORDINATE_CHECKPOINT_TAIL_OPTIC_SOURCE',
+ { context: { field: 'frontier' } },
+ );
+ }
assertNonEmpty(options.checkpointSha, 'checkpointSha');
@@
function assertNonEmpty(value: string, field: string): void {
- if (value.length === 0) {
+ if (typeof value !== 'string' || value.trim().length === 0) {
throw new WarpError(As per coding guidelines, "Validate at boundaries and constructors; constructors establish invariants and must not perform I/O."
Also applies to: 76-80, 88-90
🤖 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/CoordinateCheckpointTailOpticSource.ts` around
lines 27 - 38, The constructor currently trusts options.frontier and fields on
options.source and can throw raw TypeErrors; add defensive validation at the
start of the CoordinateCheckpointTailOpticSource constructor to assert
options.source exists and has required identity fields (graphName, _persistence,
_codec, _blobStorage, _commitMessageCodec) using assertNonEmpty where
appropriate, validate options.checkpointSha (already present) and validate
options.frontier is non-null and shape-compatible before calling copyFrontier;
replace raw errors by throwing a WarpError (with a clear message) if validation
fails, and wrap the copyFrontier call in a try/catch that converts unexpected
exceptions into a WarpError so the constructor always enforces invariants rather
than leaking TypeErrors.
Comment on lines
+136
to
+139
| throw new QueryError('v17 foundation optics support live worldlines only', { | ||
| code: 'E_OPTIC_NO_BOUNDED_BASIS', | ||
| context: { selector: this._source.constructor.name }, | ||
| }); |
There was a problem hiding this comment.
Fix stale error text in unsupported-selector branch.
This now supports coordinate selectors, but the message still says “live worldlines only,” which is misleading during error handling/debugging.
Suggested patch
- throw new QueryError('v17 foundation optics support live worldlines only', {
+ throw new QueryError('v17 foundation optics support live and coordinate worldlines only', {
code: 'E_OPTIC_NO_BOUNDED_BASIS',
context: { selector: this._source.constructor.name },
});🤖 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/Worldline.ts` around lines 136 - 139, Update the thrown
QueryError in the unsupported-selector branch to use a clear, accurate message
instead of "v17 foundation optics support live worldlines only"; locate the
throw new QueryError(...) with code 'E_OPTIC_NO_BOUNDED_BASIS' (inside the
unsupported-selector branch that references this._source.constructor.name) and
change the error text to describe the actual unsupported selector case (e.g.,
indicate that the selector type is unsupported or must be a coordinate
selector), keeping the same error code and context payload.
Comment on lines
+81
to
+86
| function validateCheckpointSha(checkpointSha: string | null | undefined): string | null { | ||
| if (checkpointSha === undefined || checkpointSha === null) { | ||
| return null; | ||
| } | ||
| if (typeof checkpointSha !== 'string' || checkpointSha.length === 0) { | ||
| throw new QueryError('checkpointSha must be a non-empty string when provided', { |
There was a problem hiding this comment.
Tighten checkpointSha validation to reject whitespace-only values.
Current validation allows " " which weakens selector invariants.
Suggested fix
- if (typeof checkpointSha !== 'string' || checkpointSha.length === 0) {
+ if (typeof checkpointSha !== 'string' || checkpointSha.trim().length === 0) {
throw new QueryError('checkpointSha must be a non-empty string when provided', {As per coding guidelines, "Validate at boundaries and constructors; constructors establish invariants and must not perform I/O."
🤖 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/types/CoordinateSelector.ts` around lines 81 - 86, The
validateCheckpointSha function currently accepts strings that are only
whitespace; update its validation in validateCheckpointSha to first ensure
checkpointSha is a string, then trim() it and reject if the trimmed length is 0
(throw the same QueryError message), returning the trimmed value (or null when
input is null/undefined) so whitespace-only inputs are treated as invalid and
the selector invariant is preserved.
Comment on lines
+25
to
+33
| constructor(options: WarpWorldlineCoordinateOptions) { | ||
| assertNonEmpty(options.worldlineName, 'worldlineName'); | ||
| assertNonEmpty(options.checkpointSha, 'checkpointSha'); | ||
| this.worldlineName = options.worldlineName; | ||
| this.checkpointSha = options.checkpointSha; | ||
| this.frontierEntries = freezeFrontier(options.frontier); | ||
| this._createWorldline = options.createWorldline; | ||
| Object.freeze(this); | ||
| } |
There was a problem hiding this comment.
Harden constructor boundary checks for frontier and createWorldline.
Invalid runtime input currently fails later with raw TypeErrors (frontier.entries / calling non-function) instead of a domain error at construction time.
Suggested fix
constructor(options: WarpWorldlineCoordinateOptions) {
+ if (!(options.frontier instanceof Map)) {
+ throw new WarpError(
+ 'WarpWorldline coordinate requires a frontier Map',
+ 'E_WARP_WORLDLINE_COORDINATE',
+ { context: { field: 'frontier' } },
+ );
+ }
+ if (typeof options.createWorldline !== 'function') {
+ throw new WarpError(
+ 'WarpWorldline coordinate requires a createWorldline function',
+ 'E_WARP_WORLDLINE_COORDINATE',
+ { context: { field: 'createWorldline' } },
+ );
+ }
assertNonEmpty(options.worldlineName, 'worldlineName');
assertNonEmpty(options.checkpointSha, 'checkpointSha');
@@
function assertNonEmpty(value: string, field: string): void {
- if (typeof value !== 'string' || value.length === 0) {
+ if (typeof value !== 'string' || value.trim().length === 0) {
throw new WarpError(As per coding guidelines, "Validate at boundaries and constructors; constructors establish invariants and must not perform I/O."
Also applies to: 52-54, 66-68
🤖 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/WarpWorldlineCoordinate.ts` around lines 25 - 33, The constructor
of WarpWorldlineCoordinate must validate the frontier and createWorldline inputs
up front: add explicit runtime checks in the constructor for options.frontier to
be a non-null object with an entries array (or whatever shape freezeFrontier
expects) and for options.createWorldline to be either undefined or a function
before calling freezeFrontier or assigning to this._createWorldline; if
validation fails throw a domain error (e.g., TypeError or a custom error) with a
clear message. Locate the constructor in class WarpWorldlineCoordinate and add
these checks prior to calling freezeFrontier(options.frontier) and before
setting this._createWorldline so invalid inputs fail fast and with a
domain-style error. Ensure the checks mirror the same validation you will add in
the other constructor uses referenced around the class (lines noted in the
review) so all construction paths enforce the invariants.
Comment on lines
+22
to
+24
| function assertNonEmpty(value: string, field: string): void { | ||
| if (typeof value !== 'string' || value.length === 0) { | ||
| throw new WarpError( |
There was a problem hiding this comment.
Reject whitespace-only identity values in constructor validation.
assertNonEmpty currently accepts " ", which allows invalid identity fields into this value object.
Suggested fix
function assertNonEmpty(value: string, field: string): void {
- if (typeof value !== 'string' || value.length === 0) {
+ if (typeof value !== 'string' || value.trim().length === 0) {
throw new WarpError(As per coding guidelines, "Validate at boundaries and constructors; constructors establish invariants and must not perform I/O."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function assertNonEmpty(value: string, field: string): void { | |
| if (typeof value !== 'string' || value.length === 0) { | |
| throw new WarpError( | |
| function assertNonEmpty(value: string, field: string): void { | |
| if (typeof value !== 'string' || value.trim().length === 0) { | |
| throw new WarpError( |
🤖 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/WarpWorldlineOpticBasis.ts` around lines 22 - 24, The
assertNonEmpty validator currently allows strings that are only whitespace;
update the function assertNonEmpty(value: string, field: string) used by
WarpWorldlineOpticBasis to treat whitespace-only values as empty by checking
value.trim().length === 0 (or using a regex) in addition to the existing typeof
check, and continue to throw WarpError(field + "...") when validation fails so
the constructor enforces the non-whitespace invariant for identity fields.
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
Verification
Summary by CodeRabbit
New Features
prepareOpticBasis(),coordinate(), andcoordinate.optic()methods.Documentation
Tests