refactor: WorldlineSelector class hierarchy (P3/P7 audit RED fix)#77
Open
flyingrobots wants to merge 22 commits intomainfrom
Open
refactor: WorldlineSelector class hierarchy (P3/P7 audit RED fix)#77flyingrobots wants to merge 22 commits intomainfrom
flyingrobots wants to merge 22 commits intomainfrom
Conversation
Four new domain types replacing the WorldlineSource discriminated union: - WorldlineSelector (abstract base, from() factory, toDTO() bridge) - LiveSelector (canonical worldline, validated ceiling) - CoordinateSelector (hypothetical worldline, private #frontier with defensive copy, validated ceiling, accepts empty frontier) - StrandSelector (isolated worldline, validated strandId + ceiling) Real extends inheritance — instanceof WorldlineSelector works. Constructor validation — rejects invalid ceiling, empty strandId, non-object frontier. Object.freeze on all instances. Map immutability via private field + defensive copy getter. 51 tests covering: instanceof, constructor validation, freeze, clone independence, toDTO, from() boundary factory, mutation isolation.
…ctor Internal consumer migration: - Worldline.js: 3 clone functions → toSelector() via WorldlineSelector.from(). materializeSource() dispatches via instanceof. source getter returns DTO via toDTO() (public API unchanged). - Observer.js: 5 clone functions → toSelector(). source getter returns DTO. _source field typed as WorldlineSelector internally. - QueryController.js: cloneObserverSource → toSelector(). Tag dispatch → instanceof LiveSelector/CoordinateSelector/StrandSelector. Public API: - WorldlineSelector, LiveSelector, CoordinateSelector, StrandSelector exported from index.js - Class declarations added to index.d.ts (additive — existing WorldlineSource interfaces unchanged) - source getters still return plain DTOs (no breaking change) Lint clean. 314 files, 5,201 tests pass.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd a WorldlineSelector abstraction (Live/Coordinate/Strand) with factory/registry and DTO bridge; migrate services to store selector instances and dispatch via runtime instanceof checks; introduce PatchJournal port and CBOR adapter, rewire patch persistence (PatchBuilderV2, Writer, Sync/Controllers) to use patchJournal; add tests and docs; update ESLint override. Changes
Sequence Diagram(s)sequenceDiagram
participant Writer as Writer
participant Builder as PatchBuilderV2
participant Journal as PatchJournalPort/CborAdapter
participant Blob as BlobPort / PatchBlobStorage
participant Persistence as GitPersistence
Writer->>Builder: builder.commit(patch)
Builder->>Journal: writePatch(patch)
alt patchJournal.usesExternalStorage
Journal->>Blob: patchBlobStorage.store(bytes) %% rgba(135,206,250,0.5)
Blob-->>Journal: oid
else
Journal->>Blob: blobPort.writeBlob(bytes) %% rgba(144,238,144,0.5)
Blob-->>Journal: oid
end
Journal-->>Builder: oid
Builder->>Persistence: write commit referencing patch oid
Persistence-->>Writer: commitSha
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/domain/services/Worldline.js (1)
214-237:⚠️ Potential issue | 🔴 CriticalFix the JSDoc return type of
sourcegetter andWorldlineSelector.toDTO()from generic object toWorldlineSourceunion.The getter and
toDTO()method are currently annotated to return{ kind: string, [key: string]: unknown }, which is too broad and prevents TypeScript from validating the types passed toobserver()calls at lines 275, 360, and 366. Since_sourceis stored asWorldlineSelectorandtoDTO()should return the specific union typeWorldlineSource(as declared inindex.d.ts), update the JSDoc annotations to match the actual contract:@returns {WorldlineSource}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/Worldline.js` around lines 214 - 237, Update the JSDoc return types so callers get the precise union type: change the `source` getter JSDoc in the Worldline class and the `toDTO()` JSDoc on `WorldlineSelector` to return `WorldlineSource` instead of the generic `{ kind: string, [key: string]: unknown }`; locate the `get source()` implementation and the `WorldlineSelector.toDTO()` declaration and replace their `@returns` annotations with `@returns {WorldlineSource}` so TypeScript can validate calls to `observer()` (referenced around `observer()` calls near lines 275, 360, 366) against the correct union type.src/domain/services/query/Observer.js (1)
258-265:⚠️ Potential issue | 🟡 MinorObserver's
sourceparameter type is inconsistently documented across methods.The constructor documents
sourceasWorldlineSource|null(line 275), but_initBacking()normalizes it viatoSelector()to aWorldlineSelector(line 265). In contrast, theseek()method already accepts the union typeWorldlineSource|WorldlineSelector|null(line 361), showing the correct pattern. This inconsistency causes the constructor's type signature to be inaccurate: it should acceptWorldlineSource | WorldlineSelector | nullto match the actual behavior of normalizing any input viatoSelector().Update the constructor's JSDoc
@paramtype forsourceto matchseek()and reflect thattoSelector()normalizes both types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/query/Observer.js` around lines 258 - 265, Update the Observer constructor JSDoc to accept the same union type as seek: change the `@param` for `source` from `WorldlineSource|null` to `WorldlineSource|WorldlineSelector|null` to reflect that the constructor normalizes via `toSelector()` (see _initBacking and toSelector) and stays consistent with seek(); ensure the comment mentions normalization by toSelector() so types align with _initBacking and seek().
🧹 Nitpick comments (1)
index.d.ts (1)
1443-1444: MarkWorldlineSelectoras abstract in the type declarationThe JSDoc comment states "Abstract base for worldline selectors," but the class declaration doesn't use the
abstractkeyword. All direct usage involves subclasses (LiveSelector,CoordinateSelector,StrandSelector), not the base class itself. Marking it abstract will prevent unintended direct instantiation and align the type surface with the documented intent.Proposed fix
-export class WorldlineSelector { +export abstract class WorldlineSelector {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.d.ts` around lines 1443 - 1444, The WorldlineSelector declaration should be marked abstract to match its JSDoc and prevent direct instantiation; update the type declaration by adding the abstract modifier to the class declaration for WorldlineSelector (ensure subclasses like LiveSelector, CoordinateSelector, StrandSelector continue to extend it and that any abstract members remain compatible).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.d.ts`:
- Around line 1453-1477: The typed API disallows passing selector class
instances even though runtime normalizes them via WorldlineSelector.from; update
the type of the source property on WorldlineOptions and ObserverOptions to
accept either the existing WorldlineSource union OR instances of the selector
classes (e.g., LiveSelector | CoordinateSelector | StrandSelector |
WorldlineSelector) so callers can pass constructed selectors; adjust the
declarations for WorldlineOptions.source and ObserverOptions.source to include
WorldlineSelector (or the explicit selector classes) in the union and keep
existing types for Backward compatibility, referencing WorldlineSelector.from
for normalization in runtime code.
In `@src/domain/services/Worldline.js`:
- Around line 15-20: materializeSource is dispatching selector class instances
(LiveSelector, CoordinateSelector) to helpers annotated to accept DTOs
(WorldlineSource), and WorldlineSource is referenced but not imported; fix this
by either converting selector instances to plain DTO objects before calling the
helpers (map properties from LiveSelector/CoordinateSelector instances into the
expected WorldlineSource shape) or update the helper signatures to accept
selector instances and explicitly extract needed properties inside helpers
(update JSDoc/types accordingly). Also add the missing import for
WorldlineSource and apply the same conversion or signature change consistently
for the other usages noted (around the constructor/initialization and the other
helper call sites referenced in the review). Ensure function names referenced
are materializeSource and the helper functions that expect WorldlineSource so
callers and types remain consistent.
In `@src/domain/types/CoordinateSelector.js`:
- Around line 29-35: The constructor's frontier validation is too loose: tighten
it in CoordinateSelector by accepting only a Map or a plain object (reject
arrays and non-plain objects via Array.isArray(frontier) and checking
Object.getPrototypeOf(frontier) === Object.prototype), and ensure all map
entries have string keys and string values before normalizing; if any key or
value is not a string, throw a TypeError. Perform these checks before the
existing normalization that sets this.#frontier (retain the existing conversion
code paths for Map and Object entries).
In `@src/domain/types/WorldlineSelector.js`:
- Line 33: The plain object registry (const registry = {}) allows prototype
properties to satisfy checks like "kind in registry", so change the registry to
a true plain map (e.g., create it with Object.create(null)) or switch all kind
existence checks to use Object.prototype.hasOwnProperty.call(registry, kind) to
avoid prototype-chain matches; update the lookup logic where registry is
referenced (including the occurrences around the constructor dispatch and the
checks noted near lines 102-104) so that only own properties are considered when
validating/dispatching by kind.
---
Outside diff comments:
In `@src/domain/services/query/Observer.js`:
- Around line 258-265: Update the Observer constructor JSDoc to accept the same
union type as seek: change the `@param` for `source` from `WorldlineSource|null`
to `WorldlineSource|WorldlineSelector|null` to reflect that the constructor
normalizes via `toSelector()` (see _initBacking and toSelector) and stays
consistent with seek(); ensure the comment mentions normalization by
toSelector() so types align with _initBacking and seek().
In `@src/domain/services/Worldline.js`:
- Around line 214-237: Update the JSDoc return types so callers get the precise
union type: change the `source` getter JSDoc in the Worldline class and the
`toDTO()` JSDoc on `WorldlineSelector` to return `WorldlineSource` instead of
the generic `{ kind: string, [key: string]: unknown }`; locate the `get
source()` implementation and the `WorldlineSelector.toDTO()` declaration and
replace their `@returns` annotations with `@returns {WorldlineSource}` so
TypeScript can validate calls to `observer()` (referenced around `observer()`
calls near lines 275, 360, 366) against the correct union type.
---
Nitpick comments:
In `@index.d.ts`:
- Around line 1443-1444: The WorldlineSelector declaration should be marked
abstract to match its JSDoc and prevent direct instantiation; update the type
declaration by adding the abstract modifier to the class declaration for
WorldlineSelector (ensure subclasses like LiveSelector, CoordinateSelector,
StrandSelector continue to extend it and that any abstract members remain
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: 795d49e3-b961-41e9-94cb-07b284ee7250
📒 Files selected for processing (11)
eslint.config.jsindex.d.tsindex.jssrc/domain/services/Worldline.jssrc/domain/services/controllers/QueryController.jssrc/domain/services/query/Observer.jssrc/domain/types/CoordinateSelector.jssrc/domain/types/LiveSelector.jssrc/domain/types/StrandSelector.jssrc/domain/types/WorldlineSelector.jstest/unit/domain/types/WorldlineSelector.test.js
Comment on lines
+29
to
+35
| if (frontier === null || frontier === undefined || typeof frontier !== 'object') { | ||
| throw new TypeError('frontier must be a Map or plain object'); | ||
| } | ||
|
|
||
| this.#frontier = frontier instanceof Map | ||
| ? new Map(frontier) | ||
| : new Map(Object.entries(frontier)); |
There was a problem hiding this comment.
Tighten frontier runtime validation before normalization
Line 29 currently accepts any object shape, including arrays/non-plain objects, and Line 33–35 allows non-string entries. That can silently create invalid coordinate selectors and break downstream assumptions about writer IDs/SHAs.
Proposed fix
constructor(frontier, ceiling) {
super();
- if (frontier === null || frontier === undefined || typeof frontier !== 'object') {
+ if (frontier === null || frontier === undefined || typeof frontier !== 'object' || Array.isArray(frontier)) {
throw new TypeError('frontier must be a Map or plain object');
}
- this.#frontier = frontier instanceof Map
- ? new Map(frontier)
- : new Map(Object.entries(frontier));
+ const isPlainObject = Object.getPrototypeOf(frontier) === Object.prototype || Object.getPrototypeOf(frontier) === null;
+ if (!(frontier instanceof Map) && !isPlainObject) {
+ throw new TypeError('frontier must be a Map or plain object');
+ }
+
+ const normalized = frontier instanceof Map
+ ? new Map(frontier)
+ : new Map(Object.entries(frontier));
+
+ for (const [writerId, sha] of normalized) {
+ if (typeof writerId !== 'string' || typeof sha !== 'string') {
+ throw new TypeError('frontier entries must be string -> string');
+ }
+ }
+ this.#frontier = normalized;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/types/CoordinateSelector.js` around lines 29 - 35, The
constructor's frontier validation is too loose: tighten it in CoordinateSelector
by accepting only a Map or a plain object (reject arrays and non-plain objects
via Array.isArray(frontier) and checking Object.getPrototypeOf(frontier) ===
Object.prototype), and ensure all map entries have string keys and string values
before normalizing; if any key or value is not a string, throw a TypeError.
Perform these checks before the existing normalization that sets this.#frontier
(retain the existing conversion code paths for Map and Object entries).
| * | ||
| * @type {Record<string, Function>} | ||
| */ | ||
| const registry = {}; |
There was a problem hiding this comment.
Harden kind lookup to avoid prototype-chain matches
Using a normal object plus kind in registry allows inherited keys to pass validation (e.g., toString), causing incorrect constructor dispatch instead of a clean “unknown kind” error.
Proposed fix
-const registry = {};
+const registry = Object.create(null);
...
- if (!(kind in registry)) {
+ if (!Object.prototype.hasOwnProperty.call(registry, kind)) {
throw new TypeError(`unknown worldline selector kind: ${String(kind)}`);
}Also applies to: 102-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/types/WorldlineSelector.js` at line 33, The plain object registry
(const registry = {}) allows prototype properties to satisfy checks like "kind
in registry", so change the registry to a true plain map (e.g., create it with
Object.create(null)) or switch all kind existence checks to use
Object.prototype.hasOwnProperty.call(registry, kind) to avoid prototype-chain
matches; update the lookup logic where registry is referenced (including the
occurrences around the constructor dispatch and the checks noted near lines
102-104) so that only own properties are considered when validating/dispatching
by kind.
Fix JSDoc type annotations so tsc can verify the selector-to-plain interface boundary: - Worldline.js: cast selector instances through unknown at materialize dispatch sites (runtime guard via instanceof) - Worldline.js: toSelector param accepts WorldlineSource union - Observer.js: cast source param at _initBacking boundary - QueryController.js: cast selector to WorldlineSource at Observer construction boundary - WorldlineSelector.js: use bracket access for index-signature properties, cast registry Ctor via JSDoc tsc: 12 errors (all pre-existing _host pattern). Zero new errors.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/domain/types/WorldlineSelector.js (1)
33-33:⚠️ Potential issue | 🟠 MajorUse own-property checks for
kindlookup.
registryis a normal object andkind in registryaccepts inherited keys likeconstructorandtoString, so invalid input can dispatch to a prototype function instead of producing the intended unknown-kind error.Proposed hardening
-const registry = {}; +const registry = Object.create(null); ... - if (!(kind in registry)) { + if (!Object.prototype.hasOwnProperty.call(registry, kind)) { throw new TypeError(`unknown worldline selector kind: ${String(kind)}`); }#!/bin/bash node - <<'NODE' const registry = {}; for (const kind of ['constructor', 'toString', '__proto__']) { try { const Ctor = registry[kind]; const value = new Ctor(); console.log(kind, 'passes_in=', kind in registry, 'ctor=', typeof Ctor, 'result=', value && value.constructor && value.constructor.name); } catch (err) { console.log(kind, 'passes_in=', kind in registry, 'ctor=', typeof registry[kind], 'error=', err.message); } } NODEAlso applies to: 102-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/types/WorldlineSelector.js` at line 33, The lookup against the plain object registry should use an own-property check instead of the inherited-key vulnerable "kind in registry" test; update the code paths that resolve a constructor from registry (the registry lookup and the dispatching logic around the symbol "registry" and the function that selects/instantiates by "kind") to first verify Object.prototype.hasOwnProperty.call(registry, kind) (or Object.hasOwn if available) and only then read registry[kind], otherwise throw the intended unknown-kind error; apply the same change to the other similar checks referenced (the block around lines 102-105) so inherited prototype keys like "constructor" or "toString" cannot be used.
🤖 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/services/Worldline.js`:
- Around line 41-45: Update the constructor JSDoc for Worldline so the `source`
parameter includes WorldlineSource DTOs (the same union used by toSelector).
Specifically, change the `@param` annotation on the Worldline constructor to
accept WorldlineSelector|WorldlineSource|{ kind: string, [key: string]: unknown
}|undefined|null (matching the toSelector signature) so TypeScript/`checkJs`
recognizes calls like `new Worldline({ source: { kind: 'live' } })`; reference
the Worldline constructor JSDoc and the toSelector(source) function to keep them
consistent.
In `@src/domain/types/WorldlineSelector.js`:
- Around line 29-33: The registry used by WorldlineSelector.from/fromPlainObject
is populated only when subclass modules import/register themselves, which makes
recognition of built-in kinds (e.g., Observer's "live" and Worldline's "strand")
dependent on module load order; fix it by eagerly registering built-ins inside
the WorldlineSelector module (or explicitly importing/initializing Observer and
Worldline registration functions) so that registry contains the default kinds
before from()/fromPlainObject() can be called—ensure you reference and call the
existing registration API (the registry object and the subclasses'
register/initialization functions) for "live" and "strand" so built-ins are
deterministically present.
---
Duplicate comments:
In `@src/domain/types/WorldlineSelector.js`:
- Line 33: The lookup against the plain object registry should use an
own-property check instead of the inherited-key vulnerable "kind in registry"
test; update the code paths that resolve a constructor from registry (the
registry lookup and the dispatching logic around the symbol "registry" and the
function that selects/instantiates by "kind") to first verify
Object.prototype.hasOwnProperty.call(registry, kind) (or Object.hasOwn if
available) and only then read registry[kind], otherwise throw the intended
unknown-kind error; apply the same change to the other similar checks referenced
(the block around lines 102-105) so inherited prototype keys like "constructor"
or "toString" cannot be used.
🪄 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: 8f3f82b3-509e-487c-ad1c-3f17ae98604e
📒 Files selected for processing (4)
src/domain/services/Worldline.jssrc/domain/services/controllers/QueryController.jssrc/domain/services/query/Observer.jssrc/domain/types/WorldlineSelector.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/domain/services/controllers/QueryController.js
Comment on lines
+41
to
+45
| * @param {WorldlineSelector|WorldlineSource|{ kind: string, [key: string]: unknown }|undefined|null} source | ||
| * @returns {import('../types/WorldlineSelector.js').default} | ||
| */ | ||
| function cloneWorldlineSource(source) { | ||
| const value = source ?? { kind: 'live' }; | ||
|
|
||
| if (value.kind === 'live') { | ||
| return cloneLiveSource(value); | ||
| } | ||
| if (value.kind === 'coordinate') { | ||
| return cloneCoordinateSource(value); | ||
| } | ||
| return { kind: 'strand', strandId: value.strandId, ceiling: value.ceiling ?? null }; | ||
| } | ||
|
|
||
| /** | ||
| * Clones a live source, preserving ceiling only if present. | ||
| * | ||
| * @param {{ kind: 'live', ceiling?: number|null }} value | ||
| * @returns {WorldlineSource} | ||
| */ | ||
| function cloneLiveSource(value) { | ||
| return 'ceiling' in value | ||
| ? { kind: 'live', ceiling: value.ceiling ?? null } | ||
| : { kind: 'live' }; | ||
| } | ||
|
|
||
| /** | ||
| * Clones a coordinate source, deep-copying the frontier. | ||
| * | ||
| * @param {{ kind: 'coordinate', frontier: Map<string, string>|Record<string, string>, ceiling?: number|null }} value | ||
| * @returns {WorldlineSource} | ||
| */ | ||
| function cloneCoordinateSource(value) { | ||
| return { | ||
| kind: 'coordinate', | ||
| frontier: value.frontier instanceof Map ? new Map(value.frontier) : { ...value.frontier }, | ||
| ceiling: value.ceiling ?? null, | ||
| }; | ||
| function toSelector(source) { | ||
| return WorldlineSelector.from(/** @type {WorldlineSelector|{ kind: string, [key: string]: unknown }|null|undefined} */ (source)).clone(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TypeScript with checkJsenabled, if a JavaScript constructor is documented as@param {{ source?: WorldlineSelector }} options, will callers passing a DTO like { source: { kind: 'live' } }be rejected even when the implementation accepts that DTO at runtime? What JSDoc union should be used to preserve backward compatibility for a parameter that accepts either a selector instance or aWorldlineSource DTO?
💡 Result:
Yes, callers passing { source: { kind: 'live' } } will be rejected by TypeScript with checkJs enabled, even if accepted at runtime. The documented type { source?: WorldlineSelector } expects source (if present) to structurally match WorldlineSelector. A nested DTO { kind: 'live' } fails structural compatibility unless WorldlineSelector has a kind property of type string. To preserve backward compatibility for a parameter accepting either a WorldlineSelector instance or WorldlineSource DTO (e.g., { kind: 'live' }), use a JSDoc union type: / * @typedef {Object} WorldlineSource * @property {string} kind */ / * @param {{ source?: WorldlineSelector | WorldlineSource }} options */ This allows callers to pass either a WorldlineSelector or the DTO for source. TypeScript's structural typing ensures { source: { kind: 'live' } } matches if WorldlineSource is defined with matching shape. Use @typedef for complex shapes to reuse and document them. Unions with | work in JSDoc for checkJs (parentheses optional for two types).
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html
- 2: JSDoc type parameter
{object|boolean}: error in object properties microsoft/TypeScript#61612 - 3: https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html
- 4: https://stackoverflow.com/questions/77048684/in-jsdoc-how-can-i-document-an-object-that-takes-both-optional-named-properties
- 5: https://stackoverflow.com/questions/65294867/jsdoc-union-type-with-objects-not-working
- 6: https://thelinuxcode.com/typescript-structural-typing-shape-safety-and-realworld-patterns/
- 7: https://stackoverflow.com/questions/16771258/how-do-you-document-jsdoc-with-mixed-parameter-type
🏁 Script executed:
# Read the constructor JSDoc section around lines 206-209
cat -n src/domain/services/Worldline.js | sed -n '200,220p'Repository: git-stunts/git-warp
Length of output: 827
🏁 Script executed:
# Check if WorldlineSource is imported
head -50 src/domain/services/Worldline.js | cat -nRepository: git-stunts/git-warp
Length of output: 2401
🏁 Script executed:
# Also check the toSelector function signature mentioned
cat -n src/domain/services/Worldline.js | sed -n '35,50p'Repository: git-stunts/git-warp
Length of output: 715
Update constructor JSDoc to accept WorldlineSource DTOs.
Line 208 documents source as WorldlineSelector only, but the implementation accepts WorldlineSource DTOs through toSelector() (line 41, 215). With checkJs enabled, TypeScript will reject valid runtime calls like new Worldline({ source: { kind: 'live' } }) even though the code supports them.
Suggested fix
- * `@param` {{ graph: WarpRuntime, source?: import('../types/WorldlineSelector.js').default }} options
+ * `@param` {{ graph: WarpRuntime, source?: import('../types/WorldlineSelector.js').default|WorldlineSource }} options🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/Worldline.js` around lines 41 - 45, Update the
constructor JSDoc for Worldline so the `source` parameter includes
WorldlineSource DTOs (the same union used by toSelector). Specifically, change
the `@param` annotation on the Worldline constructor to accept
WorldlineSelector|WorldlineSource|{ kind: string, [key: string]: unknown
}|undefined|null (matching the toSelector signature) so TypeScript/`checkJs`
recognizes calls like `new Worldline({ source: { kind: 'live' } })`; reference
the Worldline constructor JSDoc and the toSelector(source) function to keep them
consistent.
Comment on lines
+29
to
+33
| * Subclass registry — populated by each subclass module on import. | ||
| * | ||
| * @type {Record<string, Function>} | ||
| */ | ||
| const registry = {}; |
There was a problem hiding this comment.
Make built-in selector registration deterministic.
from() only recognizes subclasses that happened to register themselves earlier. With the current PR, Observer.js only guarantees live, and Worldline.js still does not guarantee strand, so plain DTO inputs can now fail with unknown worldline selector kind depending on module load order. Ensure the built-in kinds are registered before fromPlainObject() can run.
Also applies to: 83-89, 99-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/types/WorldlineSelector.js` around lines 29 - 33, The registry
used by WorldlineSelector.from/fromPlainObject is populated only when subclass
modules import/register themselves, which makes recognition of built-in kinds
(e.g., Observer's "live" and Worldline's "strand") dependent on module load
order; fix it by eagerly registering built-ins inside the WorldlineSelector
module (or explicitly importing/initializing Observer and Worldline registration
functions) so that registry contains the default kinds before
from()/fromPlainObject() can be called—ensure you reference and call the
existing registration API (the registry object and the subclasses'
register/initialization functions) for "live" and "strand" so built-ins are
deterministically present.
1. LiveSelector.toDTO() omits ceiling when null (preserves legacy 'ceiling' in dto behavior — major regression prevented) 2. Worldline.seek() removes double toSelector() call (was cloning twice unnecessarily) 3. WorldlineSelector registry freezes on first from() call (prevents post-init hijacking via _register) 4. fromPlainObject() documents hardcoded kind→args limitation 5. Two new tests: from() with missing frontier throws, frozen selector round-trips without mutation 6. toDTO null-ceiling test updated to verify key omission 5,203 tests pass. Lint clean.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/domain/types/WorldlineSelector.test.js`:
- Around line 213-219: The test cases for StrandSelector intentionally pass
invalid types and trigger TypeScript errors in CI; add a `@ts-expect-error`
comment immediately above the two invalid constructor calls in
WorldlineSelector.test.js (the lines calling new StrandSelector(123) and new
StrandSelector(null)) so the TypeScript checker ignores the expected type errors
while keeping the runtime assertions unchanged for the StrandSelector tests.
- Around line 255-265: The test calls WorldlineSelector.from(...) which returns
the base type, but then accesses subclass properties like ceiling on
LiveSelector; update the tests to narrow the type or assert the subclass before
accessing subclass members: after creating sel via WorldlineSelector.from(...)
use an instanceof check (sel instanceof LiveSelector) and move the
expect(sel.ceiling) inside that block, or use a type assertion to cast sel to
LiveSelector when asserting ceiling; reference WorldlineSelector.from,
LiveSelector and the ceiling property when applying the fix.
- Around line 44-46: The test passes an intentionally invalid string to the
LiveSelector constructor which TypeScript flags as a compile-time error; add a
ts-expect-error directive immediately above the line that constructs
LiveSelector('42') in the LiveSelector test (the it block using new
LiveSelector('42')) so the TS compiler ignores the intentional type violation
while preserving the runtime validation test.
- Around line 124-130: Add TypeScript expect-error comments to the tests that
intentionally pass invalid types so the compiler won't complain: place a
single-line "// `@ts-expect-error`" immediately above the two expect lines that
construct CoordinateSelector with null and with 'bad' (the tests referencing
CoordinateSelector), i.e., the expect(() => new CoordinateSelector(null)) and
expect(() => new CoordinateSelector('bad')) cases.
- Around line 267-295: Tests access subclass properties on a value returned by
WorldlineSelector.from without narrowing its type; cast the result to the
correct subclass before accessing subclass-only fields: for the coordinate
tests, assert or cast sel to CoordinateSelector (e.g. using TypeScript "as
CoordinateSelector" or a JSDoc /** `@type` {CoordinateSelector} */ annotation)
before reading .frontier, and for the strand test cast to StrandSelector before
reading .strandId and .ceiling so the accesses reference the correct subclass
types (symbols: WorldlineSelector.from, CoordinateSelector, StrandSelector,
.frontier, .strandId, .ceiling).
🪄 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: 77384a6a-f020-404a-aef5-f6d4910145e3
📒 Files selected for processing (4)
src/domain/services/Worldline.jssrc/domain/types/LiveSelector.jssrc/domain/types/WorldlineSelector.jstest/unit/domain/types/WorldlineSelector.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/domain/types/LiveSelector.js
- src/domain/services/Worldline.js
Comment on lines
+44
to
+46
| it('rejects string ceiling', () => { | ||
| expect(() => new LiveSelector('42')).toThrow(TypeError); | ||
| }); |
There was a problem hiding this comment.
Add @ts-expect-error to fix pipeline failure.
This test intentionally passes an invalid type to verify runtime validation, but TypeScript rejects it at compile time. The pipeline is failing because of this type error.
Proposed fix
it('rejects string ceiling', () => {
+ // `@ts-expect-error` Testing runtime validation of invalid type
expect(() => new LiveSelector('42')).toThrow(TypeError);
});📝 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
| it('rejects string ceiling', () => { | |
| expect(() => new LiveSelector('42')).toThrow(TypeError); | |
| }); | |
| it('rejects string ceiling', () => { | |
| // `@ts-expect-error` Testing runtime validation of invalid type | |
| expect(() => new LiveSelector('42')).toThrow(TypeError); | |
| }); |
🧰 Tools
🪛 GitHub Actions: Release Preflight (PR)
[error] 45-45: Typecheck failed (tsc --noEmit): TS2345 - Argument of type 'string' is not assignable to parameter of type 'number'.
🪛 GitHub Check: preflight
[failure] 45-45:
Argument of type 'string' is not assignable to parameter of type 'number'.
🪛 GitHub Check: type-firewall
[failure] 45-45:
Argument of type 'string' is not assignable to parameter of type 'number'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/types/WorldlineSelector.test.js` around lines 44 - 46, The
test passes an intentionally invalid string to the LiveSelector constructor
which TypeScript flags as a compile-time error; add a ts-expect-error directive
immediately above the line that constructs LiveSelector('42') in the
LiveSelector test (the it block using new LiveSelector('42')) so the TS compiler
ignores the intentional type violation while preserving the runtime validation
test.
Comment on lines
+124
to
+130
| it('rejects null frontier', () => { | ||
| expect(() => new CoordinateSelector(null)).toThrow(TypeError); | ||
| }); | ||
|
|
||
| it('rejects non-object frontier', () => { | ||
| expect(() => new CoordinateSelector('bad')).toThrow(TypeError); | ||
| }); |
There was a problem hiding this comment.
Add @ts-expect-error for invalid-type tests.
These tests intentionally pass invalid types to verify runtime validation but cause TypeScript errors.
Proposed fix
it('rejects null frontier', () => {
+ // `@ts-expect-error` Testing runtime validation of invalid type
expect(() => new CoordinateSelector(null)).toThrow(TypeError);
});
it('rejects non-object frontier', () => {
+ // `@ts-expect-error` Testing runtime validation of invalid type
expect(() => new CoordinateSelector('bad')).toThrow(TypeError);
});📝 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
| it('rejects null frontier', () => { | |
| expect(() => new CoordinateSelector(null)).toThrow(TypeError); | |
| }); | |
| it('rejects non-object frontier', () => { | |
| expect(() => new CoordinateSelector('bad')).toThrow(TypeError); | |
| }); | |
| it('rejects null frontier', () => { | |
| // `@ts-expect-error` Testing runtime validation of invalid type | |
| expect(() => new CoordinateSelector(null)).toThrow(TypeError); | |
| }); | |
| it('rejects non-object frontier', () => { | |
| // `@ts-expect-error` Testing runtime validation of invalid type | |
| expect(() => new CoordinateSelector('bad')).toThrow(TypeError); | |
| }); |
🧰 Tools
🪛 GitHub Check: preflight
[failure] 129-129:
Argument of type 'string' is not assignable to parameter of type 'Map<string, string> | Record<string, string>'.
[failure] 125-125:
Argument of type 'null' is not assignable to parameter of type 'Map<string, string> | Record<string, string>'.
🪛 GitHub Check: type-firewall
[failure] 129-129:
Argument of type 'string' is not assignable to parameter of type 'Map<string, string> | Record<string, string>'.
[failure] 125-125:
Argument of type 'null' is not assignable to parameter of type 'Map<string, string> | Record<string, string>'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/types/WorldlineSelector.test.js` around lines 124 - 130, Add
TypeScript expect-error comments to the tests that intentionally pass invalid
types so the compiler won't complain: place a single-line "// `@ts-expect-error`"
immediately above the two expect lines that construct CoordinateSelector with
null and with 'bad' (the tests referencing CoordinateSelector), i.e., the
expect(() => new CoordinateSelector(null)) and expect(() => new
CoordinateSelector('bad')) cases.
Comment on lines
+213
to
+219
| it('rejects non-string strandId', () => { | ||
| expect(() => new StrandSelector(123)).toThrow(TypeError); | ||
| }); | ||
|
|
||
| it('rejects null strandId', () => { | ||
| expect(() => new StrandSelector(null)).toThrow(TypeError); | ||
| }); |
There was a problem hiding this comment.
Add @ts-expect-error for invalid-type tests.
These tests intentionally pass invalid types but cause TypeScript errors in CI.
Proposed fix
it('rejects non-string strandId', () => {
+ // `@ts-expect-error` Testing runtime validation of invalid type
expect(() => new StrandSelector(123)).toThrow(TypeError);
});
it('rejects null strandId', () => {
+ // `@ts-expect-error` Testing runtime validation of invalid type
expect(() => new StrandSelector(null)).toThrow(TypeError);
});📝 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
| it('rejects non-string strandId', () => { | |
| expect(() => new StrandSelector(123)).toThrow(TypeError); | |
| }); | |
| it('rejects null strandId', () => { | |
| expect(() => new StrandSelector(null)).toThrow(TypeError); | |
| }); | |
| it('rejects non-string strandId', () => { | |
| // `@ts-expect-error` Testing runtime validation of invalid type | |
| expect(() => new StrandSelector(123)).toThrow(TypeError); | |
| }); | |
| it('rejects null strandId', () => { | |
| // `@ts-expect-error` Testing runtime validation of invalid type | |
| expect(() => new StrandSelector(null)).toThrow(TypeError); | |
| }); |
🧰 Tools
🪛 GitHub Check: preflight
[failure] 218-218:
Argument of type 'null' is not assignable to parameter of type 'string'.
[failure] 214-214:
Argument of type 'number' is not assignable to parameter of type 'string'.
🪛 GitHub Check: type-firewall
[failure] 218-218:
Argument of type 'null' is not assignable to parameter of type 'string'.
[failure] 214-214:
Argument of type 'number' is not assignable to parameter of type 'string'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/types/WorldlineSelector.test.js` around lines 213 - 219, The
test cases for StrandSelector intentionally pass invalid types and trigger
TypeScript errors in CI; add a `@ts-expect-error` comment immediately above the
two invalid constructor calls in WorldlineSelector.test.js (the lines calling
new StrandSelector(123) and new StrandSelector(null)) so the TypeScript checker
ignores the expected type errors while keeping the runtime assertions unchanged
for the StrandSelector tests.
Comment on lines
+255
to
+265
| it('converts { kind: "live" } to LiveSelector', () => { | ||
| const sel = WorldlineSelector.from({ kind: 'live' }); | ||
| expect(sel).toBeInstanceOf(LiveSelector); | ||
| expect(sel.ceiling).toBe(null); | ||
| }); | ||
|
|
||
| it('converts { kind: "live", ceiling: 42 } to LiveSelector', () => { | ||
| const sel = WorldlineSelector.from({ kind: 'live', ceiling: 42 }); | ||
| expect(sel).toBeInstanceOf(LiveSelector); | ||
| expect(sel.ceiling).toBe(42); | ||
| }); |
There was a problem hiding this comment.
Add type assertions to access subclass properties.
WorldlineSelector.from() returns WorldlineSelector, but tests access subclass-specific properties like ceiling. TypeScript needs type narrowing or assertions.
Proposed fix using type assertions
it('converts { kind: "live" } to LiveSelector', () => {
const sel = WorldlineSelector.from({ kind: 'live' });
expect(sel).toBeInstanceOf(LiveSelector);
- expect(sel.ceiling).toBe(null);
+ expect((sel as LiveSelector).ceiling).toBe(null);
});
it('converts { kind: "live", ceiling: 42 } to LiveSelector', () => {
const sel = WorldlineSelector.from({ kind: 'live', ceiling: 42 });
expect(sel).toBeInstanceOf(LiveSelector);
- expect(sel.ceiling).toBe(42);
+ expect((sel as LiveSelector).ceiling).toBe(42);
});Alternatively, narrow after the instanceof check by splitting the assertion:
const sel = WorldlineSelector.from({ kind: 'live' });
expect(sel).toBeInstanceOf(LiveSelector);
if (sel instanceof LiveSelector) {
expect(sel.ceiling).toBe(null);
}🧰 Tools
🪛 GitHub Check: preflight
[failure] 264-264:
Property 'ceiling' does not exist on type 'WorldlineSelector'.
[failure] 258-258:
Property 'ceiling' does not exist on type 'WorldlineSelector'.
🪛 GitHub Check: type-firewall
[failure] 264-264:
Property 'ceiling' does not exist on type 'WorldlineSelector'.
[failure] 258-258:
Property 'ceiling' does not exist on type 'WorldlineSelector'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/types/WorldlineSelector.test.js` around lines 255 - 265, The
test calls WorldlineSelector.from(...) which returns the base type, but then
accesses subclass properties like ceiling on LiveSelector; update the tests to
narrow the type or assert the subclass before accessing subclass members: after
creating sel via WorldlineSelector.from(...) use an instanceof check (sel
instanceof LiveSelector) and move the expect(sel.ceiling) inside that block, or
use a type assertion to cast sel to LiveSelector when asserting ceiling;
reference WorldlineSelector.from, LiveSelector and the ceiling property when
applying the fix.
Comment on lines
+267
to
+295
| it('converts { kind: "coordinate" } to CoordinateSelector', () => { | ||
| const sel = WorldlineSelector.from({ | ||
| kind: 'coordinate', | ||
| frontier: new Map([['alice', 'abc']]), | ||
| ceiling: null, | ||
| }); | ||
| expect(sel).toBeInstanceOf(CoordinateSelector); | ||
| expect(sel.frontier.get('alice')).toBe('abc'); | ||
| }); | ||
|
|
||
| it('converts { kind: "coordinate" } with plain object frontier', () => { | ||
| const sel = WorldlineSelector.from({ | ||
| kind: 'coordinate', | ||
| frontier: { alice: 'abc' }, | ||
| }); | ||
| expect(sel).toBeInstanceOf(CoordinateSelector); | ||
| expect(sel.frontier).toBeInstanceOf(Map); | ||
| }); | ||
|
|
||
| it('converts { kind: "strand" } to StrandSelector', () => { | ||
| const sel = WorldlineSelector.from({ | ||
| kind: 'strand', | ||
| strandId: 'strand-abc', | ||
| ceiling: 10, | ||
| }); | ||
| expect(sel).toBeInstanceOf(StrandSelector); | ||
| expect(sel.strandId).toBe('strand-abc'); | ||
| expect(sel.ceiling).toBe(10); | ||
| }); |
There was a problem hiding this comment.
Add type assertions for frontier and strandId access.
Same issue: subclass-specific properties accessed on base type.
Proposed fix
it('converts { kind: "coordinate" } to CoordinateSelector', () => {
const sel = WorldlineSelector.from({
kind: 'coordinate',
frontier: new Map([['alice', 'abc']]),
ceiling: null,
});
expect(sel).toBeInstanceOf(CoordinateSelector);
- expect(sel.frontier.get('alice')).toBe('abc');
+ expect((sel as CoordinateSelector).frontier.get('alice')).toBe('abc');
});
it('converts { kind: "coordinate" } with plain object frontier', () => {
const sel = WorldlineSelector.from({
kind: 'coordinate',
frontier: { alice: 'abc' },
});
expect(sel).toBeInstanceOf(CoordinateSelector);
- expect(sel.frontier).toBeInstanceOf(Map);
+ expect((sel as CoordinateSelector).frontier).toBeInstanceOf(Map);
});
it('converts { kind: "strand" } to StrandSelector', () => {
const sel = WorldlineSelector.from({
kind: 'strand',
strandId: 'strand-abc',
ceiling: 10,
});
expect(sel).toBeInstanceOf(StrandSelector);
- expect(sel.strandId).toBe('strand-abc');
- expect(sel.ceiling).toBe(10);
+ expect((sel as StrandSelector).strandId).toBe('strand-abc');
+ expect((sel as StrandSelector).ceiling).toBe(10);
});📝 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
| it('converts { kind: "coordinate" } to CoordinateSelector', () => { | |
| const sel = WorldlineSelector.from({ | |
| kind: 'coordinate', | |
| frontier: new Map([['alice', 'abc']]), | |
| ceiling: null, | |
| }); | |
| expect(sel).toBeInstanceOf(CoordinateSelector); | |
| expect(sel.frontier.get('alice')).toBe('abc'); | |
| }); | |
| it('converts { kind: "coordinate" } with plain object frontier', () => { | |
| const sel = WorldlineSelector.from({ | |
| kind: 'coordinate', | |
| frontier: { alice: 'abc' }, | |
| }); | |
| expect(sel).toBeInstanceOf(CoordinateSelector); | |
| expect(sel.frontier).toBeInstanceOf(Map); | |
| }); | |
| it('converts { kind: "strand" } to StrandSelector', () => { | |
| const sel = WorldlineSelector.from({ | |
| kind: 'strand', | |
| strandId: 'strand-abc', | |
| ceiling: 10, | |
| }); | |
| expect(sel).toBeInstanceOf(StrandSelector); | |
| expect(sel.strandId).toBe('strand-abc'); | |
| expect(sel.ceiling).toBe(10); | |
| }); | |
| it('converts { kind: "coordinate" } to CoordinateSelector', () => { | |
| const sel = WorldlineSelector.from({ | |
| kind: 'coordinate', | |
| frontier: new Map([['alice', 'abc']]), | |
| ceiling: null, | |
| }); | |
| expect(sel).toBeInstanceOf(CoordinateSelector); | |
| expect((sel as CoordinateSelector).frontier.get('alice')).toBe('abc'); | |
| }); | |
| it('converts { kind: "coordinate" } with plain object frontier', () => { | |
| const sel = WorldlineSelector.from({ | |
| kind: 'coordinate', | |
| frontier: { alice: 'abc' }, | |
| }); | |
| expect(sel).toBeInstanceOf(CoordinateSelector); | |
| expect((sel as CoordinateSelector).frontier).toBeInstanceOf(Map); | |
| }); | |
| it('converts { kind: "strand" } to StrandSelector', () => { | |
| const sel = WorldlineSelector.from({ | |
| kind: 'strand', | |
| strandId: 'strand-abc', | |
| ceiling: 10, | |
| }); | |
| expect(sel).toBeInstanceOf(StrandSelector); | |
| expect((sel as StrandSelector).strandId).toBe('strand-abc'); | |
| expect((sel as StrandSelector).ceiling).toBe(10); | |
| }); |
🧰 Tools
🪛 GitHub Check: preflight
[failure] 293-293:
Property 'strandId' does not exist on type 'WorldlineSelector'.
[failure] 283-283:
Property 'frontier' does not exist on type 'WorldlineSelector'.
[failure] 274-274:
Property 'frontier' does not exist on type 'WorldlineSelector'.
🪛 GitHub Check: type-firewall
[failure] 293-293:
Property 'strandId' does not exist on type 'WorldlineSelector'.
[failure] 283-283:
Property 'frontier' does not exist on type 'WorldlineSelector'.
[failure] 274-274:
Property 'frontier' does not exist on type 'WorldlineSelector'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/types/WorldlineSelector.test.js` around lines 267 - 295,
Tests access subclass properties on a value returned by WorldlineSelector.from
without narrowing its type; cast the result to the correct subclass before
accessing subclass-only fields: for the coordinate tests, assert or cast sel to
CoordinateSelector (e.g. using TypeScript "as CoordinateSelector" or a JSDoc /**
`@type` {CoordinateSelector} */ annotation) before reading .frontier, and for the
strand test cast to StrandSelector before reading .strandId and .ceiling so the
accesses reference the correct subclass types (symbols: WorldlineSelector.from,
CoordinateSelector, StrandSelector, .frontier, .strandId, .ceiling).
Move implementation to infrastructure/codecs/DefaultCodecAdapter.js, leave one-line re-export shim in domain/utils/defaultCodec.js. 24 domain consumers keep their existing import path. bin/ and test/ files updated to import from infrastructure directly.
Proper hexagonal fix: move implementation to infrastructure, remove all 24 domain imports, inject codec from composition root (WarpRuntime). No re-export shim — domain never touches a codec implementation.
The original item said "move defaultCodec.js to infrastructure." That treats the symptom. The disease: 20 domain services call codec.encode()/decode() directly — domain doing serialization. defaultCodec is a singleton pretending to be DI. The optional constructor param is theater — every service bypasses injection by importing the global. The fix is architectural: extract serialization from domain services to the infrastructure boundary. When no domain service needs a codec, defaultCodec becomes unnecessary. Revert of the failed cycle 0007 defaultCodec migration attempt.
…odec failed) WorldlineSelector: shipped in PR #77. Real extends, constructor validation, private #frontier, toDTO bridge, 53 tests. defaultCodec: three approaches tried, all wrong. Root cause: the problem was misdiagnosed as a file move. The real P5 violation is 20 domain services doing serialization. defaultCodec is a singleton pretending to be DI. Backlog item rewritten as L-effort architectural work.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/method/retro/0007-viewpoint-design/viewpoint-design.md (1)
93-94: Tighten wording to avoid sentence fragment.“(imported by nothing, tested but unused). Should be deleted immediately.” reads choppy. Consider merging into one complete sentence for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/method/retro/0007-viewpoint-design/viewpoint-design.md` around lines 93 - 94, The sentence fragment about canonicalCbor.js is choppy; rewrite the two fragments into one clear sentence such as: "The file canonicalCbor.js is dead code — it is not imported anywhere and, although tested, is unused; it should be deleted immediately." Update the text around the symbol canonicalCbor.js in viewpoint-design.md to replace the current two-part phrasing with this single complete sentence.
🤖 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/method/backlog/asap/NDNM_defaultcodec-to-infrastructure.md`:
- Around line 7-21: The document's offender count is inconsistent (title and
descriptions say "20 services" but the listed entries contain 21 and omit
src/domain/WarpRuntime.js); update the text so the numeric claim matches the
actual list: either remove the extra listed entry to make it 20 or include
src/domain/WarpRuntime.js in the offender list and change all mentions of "20"
to "21" (title, Problem statement, Fix section), and re-run a quick grep for
imports of `defaultCodec` to ensure the inventory is complete before using this
doc as the migration source-of-truth.
In `@docs/method/retro/0007-viewpoint-design/viewpoint-design.md`:
- Around line 7-8: Update the test-count in the retro to match the PR objective:
replace the sentence "All 5,203 tests pass." in viewpoint-design.md (the
WorldlineSelector hierarchy entry referencing PR `#77`) with the correct total
"All 5,252 tests pass." so the documented total equals 5,201 existing tests plus
51 new selector tests.
---
Nitpick comments:
In `@docs/method/retro/0007-viewpoint-design/viewpoint-design.md`:
- Around line 93-94: The sentence fragment about canonicalCbor.js is choppy;
rewrite the two fragments into one clear sentence such as: "The file
canonicalCbor.js is dead code — it is not imported anywhere and, although
tested, is unused; it should be deleted immediately." Update the text around the
symbol canonicalCbor.js in viewpoint-design.md to replace the current two-part
phrasing with this single complete sentence.
🪄 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: e6b229c4-6c58-497d-ae4c-50da1312a410
📒 Files selected for processing (2)
docs/method/backlog/asap/NDNM_defaultcodec-to-infrastructure.mddocs/method/retro/0007-viewpoint-design/viewpoint-design.md
Comment on lines
+7
to
+21
| 20 domain services import `defaultCodec` and call `codec.encode()` / | ||
| `codec.decode()` directly. This is a P5 violation: "Serialization Is | ||
| the Codec's Problem." Domain services should work with domain objects, | ||
| not bytes. Serialization belongs at the infrastructure boundary. | ||
|
|
||
| `defaultCodec` is a singleton pretending to be dependency injection. | ||
| The `codec` constructor param is theater — every service can bypass | ||
| its caller and import the global directly. | ||
|
|
||
| The original backlog item framed this as "move defaultCodec.js to | ||
| infrastructure." That's wrong. Moving the file treats the symptom. | ||
| The disease is that 20 domain services do serialization. | ||
|
|
||
| ## The 20 Offenders | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all JS files importing defaultCodec from domain utils
rg -n --type=js "import\s+defaultCodec\s+from\s+['\"].*defaultCodec\.js['\"]" -S | sort
echo "----"
echo "Total import sites:"
rg -n --type=js "import\s+defaultCodec\s+from\s+['\"].*defaultCodec\.js['\"]" -S | wc -lRepository: git-stunts/git-warp
Length of output: 4151
🏁 Script executed:
cat -n docs/method/backlog/asap/NDNM_defaultcodec-to-infrastructure.md | head -100Repository: git-stunts/git-warp
Length of output: 3114
Align offender count: document claims 20 services but lists 21 items.
The offender list (lines 22–62) contains 21 entries, while the document title (line 20), Problem statement (lines 7, 18), and Fix section (line 69) all reference "20 services." Additionally, src/domain/WarpRuntime.js imports defaultCodec but is omitted from the list. Reconcile the count and verify the offender inventory is complete before using this as the migration source-of-truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/method/backlog/asap/NDNM_defaultcodec-to-infrastructure.md` around lines
7 - 21, The document's offender count is inconsistent (title and descriptions
say "20 services" but the listed entries contain 21 and omit
src/domain/WarpRuntime.js); update the text so the numeric claim matches the
actual list: either remove the extra listed entry to make it 20 or include
src/domain/WarpRuntime.js in the offender list and change all mentions of "20"
to "21" (title, Problem statement, Fix section), and re-run a quick grep for
imports of `defaultCodec` to ensure the inventory is complete before using this
doc as the migration source-of-truth.
Comment on lines
+7
to
+8
| WorldlineSelector hierarchy: shipped (PR #77). All 5,203 tests pass. | ||
| defaultCodec migration: failed. Reverted. Backlog item rewritten. |
There was a problem hiding this comment.
Fix test-count mismatch in the retro outcome.
Line 7 says “5,203 tests pass,” but the PR objective says 5,201 existing tests plus 51 new selector tests. Please reconcile the exact total so the retro stays trustworthy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/method/retro/0007-viewpoint-design/viewpoint-design.md` around lines 7 -
8, Update the test-count in the retro to match the PR objective: replace the
sentence "All 5,203 tests pass." in viewpoint-design.md (the WorldlineSelector
hierarchy entry referencing PR `#77`) with the correct total "All 5,252 tests
pass." so the documented total equals 5,201 existing tests plus 51 new selector
tests.
The backlog item and retro both described the wrong fix: "move serialization-primary services to infrastructure." That keeps serializers alive in a different folder. The real fix: domain services produce domain objects. The persistence adapter serializes at the boundary. Serializer services dissolve into the adapter layer. Port contracts speak domain types, not bytes. defaultCodec disappears because nothing in domain needs it. Updated: - NDNM_defaultcodec-to-infrastructure.md (backlog) - Cycle 0007 retro (struck wrong redo plan, added correction) - BEARING.md (updated defaultCodec + WorldlineSelector status)
Rewrite the defaultCodec backlog item with the correct architecture: domain-facing artifact ports (PatchJournalPort, CheckpointStorePort, IndexStorePort, ProvenanceStorePort, BtrStorePort) that speak domain types, backed by codec-owning infrastructure adapters over the raw Git ports. Raw Git ports (BlobPort, TreePort, CommitPort, RefPort) stay as infrastructure primitives. Key refinements: - Don't dissolve serializers into GitGraphAdapter (god object) - Split semantic projection from byte encoding in mixed files - Strangler refactor: patches → checkpoints → indexes → provenance/BTR - Boundary records named by artifact (PatchRecord, CheckpointRecord) - Hex Tripwire Test + Golden Blob Museum as hard gates Updated backlog item, cycle 0007 retro, and BEARING.md.
- Hex Tripwire Test: scan src/domain/ for forbidden imports, ratchet down - Golden Blob Museum: canonical fixtures for wire format compatibility - Artifact Store Stack Diagram: persistence stack visualization - Serializer Exorcism Commit Series: one artifact family per commit
RED phase: all tests fail as expected. - Hex tripwire: scans PatchBuilderV2, SyncProtocol, Writer for forbidden codec imports/usage. 9 failures confirm the P5 violations exist. - PatchJournalPort contract test: fails because port doesn't exist yet. - CborPatchJournalAdapter test: fails because adapter doesn't exist yet. Includes golden fixture (known CBOR bytes) for wire format stability and encrypted patch support tests.
PatchJournalPort is a domain-facing port that speaks PatchV2 objects. No bytes cross this boundary. CborPatchJournalAdapter implements it using CborCodec + BlobPort (or BlobStoragePort for encrypted patches). This is the first artifact-level port in the two-stage persistence boundary (P5 codec dissolution, Slice 1). Port: src/ports/PatchJournalPort.js Adapter: src/infrastructure/adapters/CborPatchJournalAdapter.js
PatchBuilderV2 no longer imports defaultCodec or calls codec.encode(). Patch persistence goes through PatchJournalPort — the adapter owns the codec and blob storage. Changes: - Remove defaultCodec import from PatchBuilderV2.js - Replace codec + patchBlobStorage params with patchJournal - commit() calls patchJournal.writePatch(patch) instead of codec.encode(patch) + writeBlob(bytes) - Add usesExternalStorage getter to PatchJournalPort/adapter for the encrypted trailer flag - Update 3 tests to wire CborPatchJournalAdapter All 67 PatchBuilderV2 tests pass.
SyncProtocol no longer imports defaultCodec or calls codec.decode(). Patch blob reading goes through PatchJournalPort.readPatch() — the adapter owns the codec. Changes: - Remove defaultCodec import from SyncProtocol.js - Replace codec + patchBlobStorage params with patchJournal on loadPatchFromCommit, loadPatchRange, processSyncRequest - Remove dead EncryptionError import (now handled by adapter) - Update all 4 SyncProtocol test files to wire CborPatchJournalAdapter All 45 SyncProtocol tests pass (32 + 4 + 4 + 5).
… stack Writer.js no longer imports defaultCodec. PatchJournalPort is wired through the full call chain: WarpRuntime → PatchController → Writer → PatchBuilderV2, and WarpRuntime → SyncController → SyncProtocol. Changes: - Writer: replace codec/patchBlobStorage with patchJournal - PatchController: pass patchJournal instead of codec to Writer and PatchBuilderV2; use patchJournal.readPatch() for patch loading - SyncController: pass patchJournal instead of codec to SyncProtocol - StrandService: wire patchJournal to PatchBuilderV2 constructions - WarpRuntime: accept patchJournal param, store as _patchJournal Hex tripwire is GREEN: all 27 checks pass across PatchBuilderV2, SyncProtocol, and Writer. Zero forbidden codec patterns.
WarpRuntime constructor options are untyped, causing tsc to resolve _patchJournal as `any`. Added targeted eslint-disable-next-line comments for strict-boolean-expressions and no-unsafe-assignment in PatchBuilderV2, PatchController, and StrandService.
…lPort migration Tests for PatchBuilderV2, Writer, SyncController, and TreeConstruction.determinism were failing with "patchJournal is required for committing patches" after the PatchJournalPort extraction. Added CborPatchJournalAdapter + CborCodec imports and wired createPatchJournal helpers into every constructor that reaches commit(). SyncController test updated to assert patchJournal (not codec) in processSyncRequest delegation.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/domain/WarpRuntime.js (2)
161-363:⚠️ Potential issue | 🔴 CriticalMissing
_patchJournalproperty declaration in constructor.The constructor initializes many properties but doesn't declare
this._patchJournal. While it's set later inopen(), TypeScript requires the property to be declared in the class body or constructor for proper type checking.🔧 Fix: Add property declaration in constructor
Add after line 278 (after
_patchBlobStorage):/** `@type` {import('../ports/BlobStoragePort.js').default|null} */ this._patchBlobStorage = patchBlobStorage || null; + + /** `@type` {import('../ports/PatchJournalPort.js').default|null} */ + this._patchJournal = null;Note: This should be initialized to
nullin the constructor sinceopen()sets the actual value asynchronously. The constructor JSDoc (line 157) should also be updated to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/WarpRuntime.js` around lines 161 - 363, The constructor is missing a declaration for this._patchJournal which causes TS type errors; inside the constructor (e.g., near where _patchBlobStorage is set) add a property declaration initializing this._patchJournal = null and update the constructor JSDoc (or class JSDoc) to include the _patchJournal type so that the later assignment in open() is type-safe; reference the symbol _patchJournal, the constructor, and open() when making the change.
479-494:⚠️ Potential issue | 🔴 CriticalPipeline failure:
patchJournalnot declared in JSDoc options type.The
open()method signature (line 494) destructurespatchJournalbut the JSDoc@paramat line 481 doesn't include it in the options type. This causes the TypeScript error:Property 'patchJournal' does not exist on type '{ persistence: CorePersistence; ... }'
🔧 Fix: Add patchJournal to JSDoc `@param`
- * `@param` {{ persistence: CorePersistence, graphName: string, writerId: string, gcPolicy?: Record<string, unknown>, adjacencyCacheSize?: number, checkpointPolicy?: {every: number}, autoMaterialize?: boolean, onDeleteWithData?: 'reject'|'cascade'|'warn', logger?: import('../ports/LoggerPort.js').default, clock?: import('../ports/ClockPort.js').default, crypto?: import('../ports/CryptoPort.js').default, codec?: import('../ports/CodecPort.js').default, seekCache?: import('../ports/SeekCachePort.js').default, audit?: boolean, blobStorage?: import('../ports/BlobStoragePort.js').default, patchBlobStorage?: import('../ports/BlobStoragePort.js').default, trust?: { mode?: 'off'|'log-only'|'enforce', pin?: string|null }, effectPipeline?: import('./services/EffectPipeline.js').EffectPipeline, effectSinks?: Array<import('../ports/EffectSinkPort.js').default>, externalizationPolicy?: import('./types/ExternalizationPolicy.js').ExternalizationPolicy }} options + * `@param` {{ persistence: CorePersistence, graphName: string, writerId: string, gcPolicy?: Record<string, unknown>, adjacencyCacheSize?: number, checkpointPolicy?: {every: number}, autoMaterialize?: boolean, onDeleteWithData?: 'reject'|'cascade'|'warn', logger?: import('../ports/LoggerPort.js').default, clock?: import('../ports/ClockPort.js').default, crypto?: import('../ports/CryptoPort.js').default, codec?: import('../ports/CodecPort.js').default, seekCache?: import('../ports/SeekCachePort.js').default, audit?: boolean, blobStorage?: import('../ports/BlobStoragePort.js').default, patchBlobStorage?: import('../ports/BlobStoragePort.js').default, patchJournal?: import('../ports/PatchJournalPort.js').default, trust?: { mode?: 'off'|'log-only'|'enforce', pin?: string|null }, effectPipeline?: import('./services/EffectPipeline.js').EffectPipeline, effectSinks?: Array<import('../ports/EffectSinkPort.js').default>, externalizationPolicy?: import('./types/ExternalizationPolicy.js').ExternalizationPolicy }} options🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/WarpRuntime.js` around lines 479 - 494, The JSDoc for WarpRuntime.open is missing the patchJournal option while the method signature destructures patchJournal; update the `@param` options type in the JSDoc to include patchJournal (e.g., add ", patchJournal?: import('...').default" or the appropriate type) so the options object type matches the function signature and resolves the TypeScript error for WarpRuntime.open and the patchJournal parameter.
🧹 Nitpick comments (2)
test/unit/domain/services/SyncProtocol.divergence.test.js (1)
57-62: Helper missing JSDoc documentation.For consistency with the same helper in
SyncProtocol.test.js(lines 97-107), consider adding JSDoc to document the function's purpose and return type.📝 Suggested JSDoc addition
+/** + * Creates a CborPatchJournalAdapter wired to the given mock persistence's blob ops. + * `@param` {ReturnType<typeof createMockPersistence>} persistence + * `@returns` {CborPatchJournalAdapter} + */ function createPatchJournal(persistence) { return new CborPatchJournalAdapter({ codec: new CborCodec(), blobPort: persistence, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/SyncProtocol.divergence.test.js` around lines 57 - 62, Add a JSDoc comment for the helper function createPatchJournal that documents its purpose (constructs a CborPatchJournalAdapter using a provided persistence/blob port) and its return type, referencing the CborPatchJournalAdapter and CborCodec usage and the blobPort parameter (persistence). Place the JSDoc immediately above the createPatchJournal declaration, include a short description, `@param` {Type} persistence description for the blob port, and `@returns` {CborPatchJournalAdapter} to match the style used in SyncProtocol.test.js.src/domain/services/controllers/SyncController.js (1)
40-58: Missing_patchJournalin SyncHost typedef.The
SyncHosttypedef documents the WarpRuntime surface that SyncController accesses, but_patchJournalis not declared. While the runtime behavior is safe (WarpRuntime.open() unconditionally initializes_patchJournal), adding it to the typedef would improve type safety and IDE support.📝 Suggested typedef addition
* `@typedef` {Object} SyncHost * `@property` {import('../JoinReducer.js').WarpStateV5|null} _cachedState * `@property` {Map<string, string>|null} _lastFrontier * `@property` {boolean} _stateDirty * `@property` {number} _patchesSinceGC * `@property` {string} _graphName * `@property` {CorePersistence} _persistence * `@property` {import('../../../ports/ClockPort.js').default} _clock * `@property` {import('../../../ports/CodecPort.js').default} _codec * `@property` {import('../../../ports/CryptoPort.js').default} _crypto * `@property` {import('../../../ports/LoggerPort.js').default|null} _logger * `@property` {import('../../../ports/BlobStoragePort.js').default|null} [_patchBlobStorage] +* `@property` {import('../../../ports/PatchJournalPort.js').default} _patchJournal * `@property` {number} _patchesSinceCheckpoint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/controllers/SyncController.js` around lines 40 - 58, Add the missing _patchJournal property to the SyncHost typedef in SyncController.js: declare a non-null typed property named _patchJournal (e.g. _patchJournal: import('../PatchJournal.js').default or the actual PatchJournal/CorePatchJournal type used by the runtime) so IDEs and type checks see that SyncController expects this initialized journal; update the SyncHost typedef block to include this property consistent with the other properties.
🤖 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/method/backlog/cool-ideas/DX_artifact-store-stack-diagram.md`:
- Around line 5-17: The fenced diagram block is missing a language label which
triggers markdownlint MD040; update the code fence surrounding the diagram (the
triple-backtick block that begins with "Domain Service" and contains lines like
"Artifact Port (PatchJournalPort, CheckpointStorePort, ...)" and
"GitGraphAdapter") to include a language label (use `text`) immediately after
the opening ``` so the fence becomes ```text and linting will pass.
In `@docs/method/retro/0007-viewpoint-design/viewpoint-design.md`:
- Around line 83-87: Update the “Delete dead code (canonicalCbor.js)” item to
clarify that canonicalCbor.js is not referenced by production code but is still
imported by tests (test/unit/domain/utils/canonicalCbor.test.js), so don’t
instruct outright deletion; instead change the language to “remove from
production build / mark as test-only” and update related steps (including the
“When no domain service imports defaultCodec, delete it” step) to condition
deletion on removing all test imports or migrating tests, and reference
canonicalCbor.js and defaultCodec in the guidance so readers know those are the
symbols whose usage must be audited before deletion.
In `@src/domain/services/controllers/PatchController.js`:
- Around line 164-169: The code casts h._patchJournal to a PatchJournalPort and
calls journal.readPatch without validating h._patchJournal; add a defensive
null/undefined check for h._patchJournal before the cast so you either throw a
clear error or return early if it's missing, then proceed to assign const
journal = (cast) h._patchJournal and call journal.readPatch; reference
h._patchJournal, the journal variable and journal.readPatch in your change so
the guard is colocated with that assignment in PatchController.js.
- Around line 56-57: The WarpRuntime class needs a formal JSDoc-annotated
property declaration for _patchJournal in its constructor so TypeScript
recognizes the field before it is assigned dynamically; add a line in the
WarpRuntime constructor that sets this._patchJournal = null with the JSDoc type
/** `@type` {import('../ports/PatchJournalPort.js').default|null} */ to declare
the type, which will satisfy tsc when PatchController accesses h._patchJournal
and allow the later dynamic assignment in the factory method without needing
eslint overrides.
In `@src/domain/services/PatchBuilderV2.js`:
- Around line 984-988: PatchBuilderV2 currently calls
this._patchJournal.writePatch(patch) and treats its return value as a Git blob
OID (patchBlobOid) which is wrong if the port returns an opaque storage handle;
update the port contract and the caller to be explicit: change
PatchJournalPort.writePatch to return the exact tree-entry metadata the builder
needs (e.g., { mode: '100644', type: 'blob', oid: '<git-blob-oid>', filename:
'patch.cbor' }) or else guarantee it returns a Git blob OID string, then update
the call in PatchBuilderV2 to consume that structured response (or confirm it's
an OID) before inserting into the commit tree; reference the writePatch method
and the local variable patchBlobOid/_patchJournal when making this change so the
builder and adapter agree on the returned shape.
In `@src/domain/services/strand/StrandService.js`:
- Around line 1322-1323: Queued intents are still serialized via the old
byte-path because _commitQueuedPatch() manually calls this._graph._codec.encode
and builds _content_* blob entries instead of using the shared journal/commit
flow; change _commitQueuedPatch() (and any code path from queueIntent()/tick())
to invoke the same commit helper used by PatchBuilderV2.commit() (or call
PatchBuilderV2 and its commit method) and pass through the configured
patchJournal (this._graph._patchJournal) so the PatchJournalPort is honored;
remove the inline serialization and artifact-tree hand-build in
_commitQueuedPatch(), routing serialization and artifact construction to the
shared helper so queued intents produce the same tree entries as the normal
PatchBuilderV2.commit() flow.
In `@src/domain/warp/Writer.js`:
- Around line 49-53: Writer currently treats patchJournal as optional (the
['_patchJournal','patchJournal'] mapping) but PatchBuilderV2.commit() now
hard-fails when it's missing; make patchJournal a required dependency by
updating the Writer initialization/mapping so _patchJournal/patchJournal is
required and add an explicit early check in Writer.beginPatch() that throws or
rejects immediately if this._patchJournal is absent (instead of letting
beginPatch succeed and failing later in PatchBuilderV2.commit()); ensure error
messages reference patchJournal so callers see the misconfiguration up front.
In `@src/infrastructure/adapters/CborPatchJournalAdapter.js`:
- Around line 56-63: readPatch currently falls back to this._blobPort.readBlob
when encrypted=true but this._patchBlobStorage is absent; change it to fail fast
by checking if (encrypted && !this._patchBlobStorage) and throwing a descriptive
error instead of calling this._blobPort.readBlob. Update the readPatch function
to raise a clear configuration error mentioning encrypted=true and missing
_patchBlobStorage, so callers get a direct configuration exception rather than a
misleading blob-corrupt/missing error.
In `@test/unit/boundary/patch-codec-tripwire.test.js`:
- Around line 23-27: The tripwire only lists PATCH_FILES and regex patterns that
miss nested codec accesses; update PATCH_FILES to include
'src/domain/services/strand/StrandService.js' (and any other domain files that
may call codec via nested paths) and broaden the regexes (the pattern set
referenced around lines 33-43) to detect nested access like
this._graph._codec.encode/decode by matching any property chain before _codec
(e.g. allow .<identifier>*. _codec\.(encode|decode) or equivalent), so the test
fails if any nested ._codec.encode or ._codec.decode usage remains outside the
allowed patch files.
---
Outside diff comments:
In `@src/domain/WarpRuntime.js`:
- Around line 161-363: The constructor is missing a declaration for
this._patchJournal which causes TS type errors; inside the constructor (e.g.,
near where _patchBlobStorage is set) add a property declaration initializing
this._patchJournal = null and update the constructor JSDoc (or class JSDoc) to
include the _patchJournal type so that the later assignment in open() is
type-safe; reference the symbol _patchJournal, the constructor, and open() when
making the change.
- Around line 479-494: The JSDoc for WarpRuntime.open is missing the
patchJournal option while the method signature destructures patchJournal; update
the `@param` options type in the JSDoc to include patchJournal (e.g., add ",
patchJournal?: import('...').default" or the appropriate type) so the options
object type matches the function signature and resolves the TypeScript error for
WarpRuntime.open and the patchJournal parameter.
---
Nitpick comments:
In `@src/domain/services/controllers/SyncController.js`:
- Around line 40-58: Add the missing _patchJournal property to the SyncHost
typedef in SyncController.js: declare a non-null typed property named
_patchJournal (e.g. _patchJournal: import('../PatchJournal.js').default or the
actual PatchJournal/CorePatchJournal type used by the runtime) so IDEs and type
checks see that SyncController expects this initialized journal; update the
SyncHost typedef block to include this property consistent with the other
properties.
In `@test/unit/domain/services/SyncProtocol.divergence.test.js`:
- Around line 57-62: Add a JSDoc comment for the helper function
createPatchJournal that documents its purpose (constructs a
CborPatchJournalAdapter using a provided persistence/blob port) and its return
type, referencing the CborPatchJournalAdapter and CborCodec usage and the
blobPort parameter (persistence). Place the JSDoc immediately above the
createPatchJournal declaration, include a short description, `@param` {Type}
persistence description for the blob port, and `@returns`
{CborPatchJournalAdapter} to match the style used in SyncProtocol.test.js.
🪄 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: 4143621e-469c-4a23-9ac5-3bfaf48e2f98
📒 Files selected for processing (28)
docs/BEARING.mddocs/method/backlog/asap/NDNM_defaultcodec-to-infrastructure.mddocs/method/backlog/cool-ideas/DX_artifact-store-stack-diagram.mddocs/method/backlog/cool-ideas/DX_golden-blob-museum.mddocs/method/backlog/cool-ideas/DX_hex-tripwire-test.mddocs/method/backlog/cool-ideas/DX_serializer-exorcism-commit-series.mddocs/method/retro/0007-viewpoint-design/viewpoint-design.mdsrc/domain/WarpRuntime.jssrc/domain/services/PatchBuilderV2.jssrc/domain/services/controllers/PatchController.jssrc/domain/services/controllers/SyncController.jssrc/domain/services/strand/StrandService.jssrc/domain/services/sync/SyncProtocol.jssrc/domain/warp/Writer.jssrc/infrastructure/adapters/CborPatchJournalAdapter.jssrc/ports/PatchJournalPort.jstest/unit/boundary/patch-codec-tripwire.test.jstest/unit/domain/services/PatchBuilderV2.cas.test.jstest/unit/domain/services/PatchBuilderV2.content.test.jstest/unit/domain/services/PatchBuilderV2.test.jstest/unit/domain/services/SyncController.test.jstest/unit/domain/services/SyncProtocol.divergence.test.jstest/unit/domain/services/SyncProtocol.stateCoherence.test.jstest/unit/domain/services/SyncProtocol.test.jstest/unit/domain/services/TreeConstruction.determinism.test.jstest/unit/domain/warp/Writer.test.jstest/unit/infrastructure/adapters/CborPatchJournalAdapter.test.jstest/unit/ports/PatchJournalPort.test.js
✅ Files skipped from review due to trivial changes (5)
- test/unit/ports/PatchJournalPort.test.js
- docs/BEARING.md
- docs/method/backlog/cool-ideas/DX_serializer-exorcism-commit-series.md
- docs/method/backlog/cool-ideas/DX_hex-tripwire-test.md
- docs/method/backlog/cool-ideas/DX_golden-blob-museum.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/method/backlog/asap/NDNM_defaultcodec-to-infrastructure.md
Comment on lines
+5
to
+17
| ``` | ||
| Domain Service | ||
| ↓ domain objects | ||
| Artifact Port (PatchJournalPort, CheckpointStorePort, ...) | ||
| ↓ domain objects | ||
| Codec Adapter (CborPatchJournalAdapter, ...) | ||
| ↓ bytes | ||
| Raw Git Port (BlobPort, TreePort, CommitPort, RefPort) | ||
| ↓ bytes | ||
| GitGraphAdapter | ||
| ↓ git plumbing calls | ||
| Git | ||
| ``` |
There was a problem hiding this comment.
Label the diagram fence.
This block is missing a fence language, so markdownlint will flag MD040 on the new doc. text is enough here.
📝 Minimal fix
-```
+```text
Domain Service
↓ domain objects
Artifact Port (PatchJournalPort, CheckpointStorePort, ...)
↓ domain objects
Codec Adapter (CborPatchJournalAdapter, ...)
↓ bytes
Raw Git Port (BlobPort, TreePort, CommitPort, RefPort)
↓ bytes
GitGraphAdapter
↓ git plumbing calls
Git
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/method/backlog/cool-ideas/DX_artifact-store-stack-diagram.md` around
lines 5 - 17, The fenced diagram block is missing a language label which
triggers markdownlint MD040; update the code fence surrounding the diagram (the
triple-backtick block that begins with "Domain Service" and contains lines like
"Artifact Port (PatchJournalPort, CheckpointStorePort, ...)" and
"GitGraphAdapter") to include a language label (use `text`) immediately after
the opening ``` so the fence becomes ```text and linting will pass.
Comment on lines
+56
to
+57
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- WarpRuntime options are untyped; cast narrows | ||
| ...(h._patchJournal !== null && h._patchJournal !== undefined ? { patchJournal: /** @type {import('../../../ports/PatchJournalPort.js').default} */ (h._patchJournal) } : {}), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify _patchJournal is not declared in WarpRuntime constructor
echo "=== Checking WarpRuntime constructor for _patchJournal declaration ==="
rg -n '_patchJournal' src/domain/WarpRuntime.js | head -20
echo ""
echo "=== Checking index.d.ts for WarpRuntime type definition ==="
rg -n '_patchJournal|patchJournal' index.d.ts 2>/dev/null || echo "No index.d.ts found or no matches"Repository: git-stunts/git-warp
Length of output: 400
🏁 Script executed:
# Check the WarpRuntime class definition and surrounding context
echo "=== Reading WarpRuntime.js constructor area ==="
sed -n '530,560p' src/domain/WarpRuntime.js
echo ""
echo "=== Searching for class declaration and property definitions ==="
head -100 src/domain/WarpRuntime.js | grep -A 5 "class WarpRuntime"Repository: git-stunts/git-warp
Length of output: 2279
🏁 Script executed:
# Check for type definition files
echo "=== Looking for .d.ts or type definition files ==="
fd -e "\.d\.ts$"
fd -e "\.js$" | xargs grep -l "WarpRuntime" | head -10Repository: git-stunts/git-warp
Length of output: 113
🏁 Script executed:
# Check PatchController.js context for lines 56-57 and also-applies-to lines
echo "=== PatchController.js lines 50-65 ==="
sed -n '50,65p' src/domain/services/controllers/PatchController.js
echo ""
echo "=== PatchController.js lines 160-170 ==="
sed -n '160,170p' src/domain/services/controllers/PatchController.js
echo ""
echo "=== PatchController.js lines 290-300 ==="
sed -n '290,300p' src/domain/services/controllers/PatchController.jsRepository: git-stunts/git-warp
Length of output: 2808
🏁 Script executed:
# Find the class definition more thoroughly
echo "=== Searching for class WarpRuntime definition ==="
rg -n "class WarpRuntime" src/domain/WarpRuntime.js -A 30
echo ""
echo "=== Searching for constructor definition ==="
rg -n "constructor\s*\(" src/domain/WarpRuntime.js -A 20 | head -60Repository: git-stunts/git-warp
Length of output: 2400
🏁 Script executed:
# Check for any property declarations in the class
echo "=== Looking for property declarations ==="
rg -n "this\._patchJournal|_patchJournal\s*=" src/domain/WarpRuntime.js | head -30Repository: git-stunts/git-warp
Length of output: 276
🏁 Script executed:
# Look at the beginning of WarpRuntime.js for context
echo "=== First 50 lines of WarpRuntime.js ==="
head -50 src/domain/WarpRuntime.jsRepository: git-stunts/git-warp
Length of output: 2735
🏁 Script executed:
# Get the full constructor to see all property initializations
echo "=== Full constructor body (lines 161-300) ==="
sed -n '161,300p' src/domain/WarpRuntime.js | head -100Repository: git-stunts/git-warp
Length of output: 3116
🏁 Script executed:
# Check what the assignments at 543 and 548 are part of
echo "=== Context around lines 543-548 (what function/method is this?) ==="
sed -n '530,560p' src/domain/WarpRuntime.jsRepository: git-stunts/git-warp
Length of output: 2234
Declare _patchJournal as a property in the WarpRuntime constructor.
The _patchJournal property is assigned dynamically after instantiation (outside the constructor) but is never formally declared in the WarpRuntime class constructor. This causes tsc --noEmit to fail when PatchController.js accesses h._patchJournal at lines 56–57, 166, and 296–297.
Add a JSDoc-annotated declaration in the constructor:
/** `@type` {import('../ports/PatchJournalPort.js').default|null} */
this._patchJournal = null;This will satisfy TypeScript's type checking and allow the dynamic assignment in the factory method to proceed without ESLint overrides.
🧰 Tools
🪛 GitHub Check: preflight
[failure] 57-57:
Property '_patchJournal' does not exist on type 'WarpRuntime'.
[failure] 57-57:
Property '_patchJournal' does not exist on type 'WarpRuntime'.
[failure] 57-57:
Property '_patchJournal' does not exist on type 'WarpRuntime'.
🪛 GitHub Check: type-firewall
[failure] 57-57:
Property '_patchJournal' does not exist on type 'WarpRuntime'.
[failure] 57-57:
Property '_patchJournal' does not exist on type 'WarpRuntime'.
[failure] 57-57:
Property '_patchJournal' does not exist on type 'WarpRuntime'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/controllers/PatchController.js` around lines 56 - 57, The
WarpRuntime class needs a formal JSDoc-annotated property declaration for
_patchJournal in its constructor so TypeScript recognizes the field before it is
assigned dynamically; add a line in the WarpRuntime constructor that sets
this._patchJournal = null with the JSDoc type /** `@type`
{import('../ports/PatchJournalPort.js').default|null} */ to declare the type,
which will satisfy tsc when PatchController accesses h._patchJournal and allow
the later dynamic assignment in the factory method without needing eslint
overrides.
Comment on lines
+164
to
+169
| /** @type {import('../../../ports/PatchJournalPort.js').default} */ | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- WarpRuntime options are untyped; cast narrows | ||
| const journal = /** @type {import('../../../ports/PatchJournalPort.js').default} */ (h._patchJournal); | ||
| const decoded = /** @type {import('../../types/WarpTypesV2.js').PatchV2} */ ( | ||
| await journal.readPatch(patchMeta.patchOid, { encrypted: patchMeta.encrypted }) | ||
| ); |
There was a problem hiding this comment.
Missing null check before using _patchJournal.
The code casts h._patchJournal directly to PatchJournalPort without a null check. While WarpRuntime.open() unconditionally initializes _patchJournal, a defensive guard would prevent a confusing runtime error if this invariant is ever violated.
🛡️ Suggested defensive check
+ if (!h._patchJournal) {
+ throw new PersistenceError(
+ 'patchJournal is required for loading patches',
+ PersistenceError.E_MISSING_OBJECT,
+ { context: { sha: currentSha } },
+ );
+ }
/** `@type` {import('../../../ports/PatchJournalPort.js').default} */
// eslint-disable-next-line `@typescript-eslint/no-unsafe-assignment` -- WarpRuntime options are untyped; cast narrows
const journal = /** `@type` {import('../../../ports/PatchJournalPort.js').default} */ (h._patchJournal);🧰 Tools
🪛 GitHub Check: preflight
[failure] 166-166:
Property '_patchJournal' does not exist on type 'WarpRuntime'.
🪛 GitHub Check: type-firewall
[failure] 166-166:
Property '_patchJournal' does not exist on type 'WarpRuntime'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/controllers/PatchController.js` around lines 164 - 169,
The code casts h._patchJournal to a PatchJournalPort and calls journal.readPatch
without validating h._patchJournal; add a defensive null/undefined check for
h._patchJournal before the cast so you either throw a clear error or return
early if it's missing, then proceed to assign const journal = (cast)
h._patchJournal and call journal.readPatch; reference h._patchJournal, the
journal variable and journal.readPatch in your change so the guard is colocated
with that assignment in PatchController.js.
Comment on lines
+984
to
+988
| // 6. Persist patch via PatchJournalPort (adapter owns encoding). | ||
| if (this._patchJournal === null || this._patchJournal === undefined) { | ||
| throw new Error('patchJournal is required for committing patches'); | ||
| } | ||
| const patchBlobOid = await this._patchJournal.writePatch(patch); |
There was a problem hiding this comment.
writePatch() still needs a Git blob OID here.
PatchJournalPort.writePatch() is introduced as returning an opaque storage handle, but Line 993 still injects this value into the commit tree as 100644 blob ${patchBlobOid}\tpatch.cbor. That means the commit path still depends on a Git blob object ID, not an arbitrary handle. Please tighten the port contract or return the tree-entry metadata the builder actually needs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/PatchBuilderV2.js` around lines 984 - 988, PatchBuilderV2
currently calls this._patchJournal.writePatch(patch) and treats its return value
as a Git blob OID (patchBlobOid) which is wrong if the port returns an opaque
storage handle; update the port contract and the caller to be explicit: change
PatchJournalPort.writePatch to return the exact tree-entry metadata the builder
needs (e.g., { mode: '100644', type: 'blob', oid: '<git-blob-oid>', filename:
'patch.cbor' }) or else guarantee it returns a Git blob OID string, then update
the call in PatchBuilderV2 to consume that structured response (or confirm it's
an OID) before inserting into the commit tree; reference the writePatch method
and the local variable patchBlobOid/_patchJournal when making this change so the
builder and adapter agree on the returned shape.
Comment on lines
+1322
to
+1323
| // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions, @typescript-eslint/no-unsafe-assignment -- WarpRuntime options are untyped; cast narrows | ||
| if (this._graph._patchJournal) { intentPbOpts.patchJournal = this._graph._patchJournal; } |
There was a problem hiding this comment.
Queued intents still persist patches through the old byte path.
Passing patchJournal into the builder here doesn’t change the path that tick() actually uses. Line 1982 onward in _commitQueuedPatch() still does this._graph._codec.encode(...) and hand-builds the artifact tree, including _content_* as blob entries instead of the tree entries used by the normal PatchBuilderV2.commit() flow. That leaves queueIntent()/tick() on a different format and bypasses any custom PatchJournalPort. Please route _commitQueuedPatch() through the same shared journal/commit helper instead of serializing the patch inline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/strand/StrandService.js` around lines 1322 - 1323, Queued
intents are still serialized via the old byte-path because _commitQueuedPatch()
manually calls this._graph._codec.encode and builds _content_* blob entries
instead of using the shared journal/commit flow; change _commitQueuedPatch()
(and any code path from queueIntent()/tick()) to invoke the same commit helper
used by PatchBuilderV2.commit() (or call PatchBuilderV2 and its commit method)
and pass through the configured patchJournal (this._graph._patchJournal) so the
PatchJournalPort is honored; remove the inline serialization and artifact-tree
hand-build in _commitQueuedPatch(), routing serialization and artifact
construction to the shared helper so queued intents produce the same tree
entries as the normal PatchBuilderV2.commit() flow.
Comment on lines
+49
to
53
| ['_patchJournal', 'patchJournal'], | ||
| ['_logger', 'logger'], | ||
| ['_onCommitSuccess', 'onCommitSuccess'], | ||
| ['_blobStorage', 'blobStorage'], | ||
| ['_patchBlobStorage', 'patchBlobStorage'], | ||
| ]); |
There was a problem hiding this comment.
Don't keep patchJournal optional if Writer can't commit without it.
Writer still models and forwards patchJournal as optional, but PatchBuilderV2.commit() now hard-fails when it's missing. That lets beginPatch() succeed and only surfaces the misconfiguration during commit, which is a breaking late failure for direct Writer consumers. Either require patchJournal here or reject the missing dependency before returning a patch session.
Also applies to: 78-85, 91-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/warp/Writer.js` around lines 49 - 53, Writer currently treats
patchJournal as optional (the ['_patchJournal','patchJournal'] mapping) but
PatchBuilderV2.commit() now hard-fails when it's missing; make patchJournal a
required dependency by updating the Writer initialization/mapping so
_patchJournal/patchJournal is required and add an explicit early check in
Writer.beginPatch() that throws or rejects immediately if this._patchJournal is
absent (instead of letting beginPatch succeed and failing later in
PatchBuilderV2.commit()); ensure error messages reference patchJournal so
callers see the misconfiguration up front.
Comment on lines
+56
to
+63
| async readPatch(patchOid, { encrypted = false } = {}) { | ||
| /** @type {Uint8Array} */ | ||
| let bytes; | ||
| if (encrypted && this._patchBlobStorage) { | ||
| bytes = await this._patchBlobStorage.retrieve(patchOid); | ||
| } else { | ||
| bytes = await this._blobPort.readBlob(patchOid); | ||
| } |
There was a problem hiding this comment.
Throw when encrypted=true but no external store is configured.
Falling back to blobPort.readBlob() on Line 62 violates the encrypted-patch contract and turns a configuration error into a misleading “missing/corrupt blob” failure. This branch should fail fast when encrypted is true and this._patchBlobStorage is null.
Suggested fix
async readPatch(patchOid, { encrypted = false } = {}) {
/** `@type` {Uint8Array} */
let bytes;
- if (encrypted && this._patchBlobStorage) {
- bytes = await this._patchBlobStorage.retrieve(patchOid);
- } else {
+ if (encrypted) {
+ if (this._patchBlobStorage === null) {
+ throw new Error('Cannot read encrypted patch without patchBlobStorage');
+ }
+ bytes = await this._patchBlobStorage.retrieve(patchOid);
+ } else {
bytes = await this._blobPort.readBlob(patchOid);
}📝 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
| async readPatch(patchOid, { encrypted = false } = {}) { | |
| /** @type {Uint8Array} */ | |
| let bytes; | |
| if (encrypted && this._patchBlobStorage) { | |
| bytes = await this._patchBlobStorage.retrieve(patchOid); | |
| } else { | |
| bytes = await this._blobPort.readBlob(patchOid); | |
| } | |
| async readPatch(patchOid, { encrypted = false } = {}) { | |
| /** `@type` {Uint8Array} */ | |
| let bytes; | |
| if (encrypted) { | |
| if (this._patchBlobStorage === null) { | |
| throw new Error('Cannot read encrypted patch without patchBlobStorage'); | |
| } | |
| bytes = await this._patchBlobStorage.retrieve(patchOid); | |
| } else { | |
| bytes = await this._blobPort.readBlob(patchOid); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/infrastructure/adapters/CborPatchJournalAdapter.js` around lines 56 - 63,
readPatch currently falls back to this._blobPort.readBlob when encrypted=true
but this._patchBlobStorage is absent; change it to fail fast by checking if
(encrypted && !this._patchBlobStorage) and throwing a descriptive error instead
of calling this._blobPort.readBlob. Update the readPatch function to raise a
clear configuration error mentioning encrypted=true and missing
_patchBlobStorage, so callers get a direct configuration exception rather than a
misleading blob-corrupt/missing error.
Comment on lines
+23
to
+27
| const PATCH_FILES = [ | ||
| 'src/domain/services/PatchBuilderV2.js', | ||
| 'src/domain/services/sync/SyncProtocol.js', | ||
| 'src/domain/warp/Writer.js', | ||
| ]; |
There was a problem hiding this comment.
This tripwire still misses nested codec calls.
Even if src/domain/services/strand/StrandService.js gets added to PATCH_FILES, these regexes still won’t match its this._graph._codec.encode(...) path, so the test can pass while a domain patch-persistence flow is still speaking bytes. Please expand both the file list and the pattern set to cover nested ._codec.(encode|decode) access before relying on this as the boundary guard.
Also applies to: 33-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/boundary/patch-codec-tripwire.test.js` around lines 23 - 27, The
tripwire only lists PATCH_FILES and regex patterns that miss nested codec
accesses; update PATCH_FILES to include
'src/domain/services/strand/StrandService.js' (and any other domain files that
may call codec via nested paths) and broaden the regexes (the pattern set
referenced around lines 33-43) to detect nested access like
this._graph._codec.encode/decode by matching any property chain before _codec
(e.g. allow .<identifier>*. _codec\.(encode|decode) or equivalent), so the test
fails if any nested ._codec.encode or ._codec.decode usage remains outside the
allowed patch files.
…RED) RED phase: all checkpoint tests fail as expected. - Tripwire extended: 4 checkpoint files (CheckpointService, CheckpointSerializerV5, StateSerializerV5, Frontier) added to scan. 7 failures confirm P5 violations exist. - CheckpointStorePort contract test: fails (port doesn't exist yet). - CborCheckpointStoreAdapter test: fails (adapter doesn't exist yet). Golden fixtures for state, appliedVV, and frontier wire format. - Refactored tripwire into reusable tripwireSuite() function.
CheckpointStorePort is a domain-facing port that speaks WarpStateV5, VersionVector, and Frontier domain objects. CborCheckpointStoreAdapter implements it using CborCodec + BlobPort + CryptoPort. Port methods: writeState/readState, writeAppliedVV/readAppliedVV, writeFrontier/readFrontier, computeStateHash. Serialization logic (orsetSerialize, vvSerialize, props/edgeBirth arrays, LWW registers) absorbed from CheckpointSerializerV5 into the adapter. Domain projection (projectStateV5) imported from domain. All 16 contract + golden fixture tests pass. Lint clean.
Strangler refactor (Slice 2): CheckpointService, CheckpointController, MaterializeController, and WarpRuntime now accept an optional checkpointStore parameter. When provided, serialization delegates to the port; when absent, the legacy codec+writeBlob path runs unchanged. WarpRuntime.open() auto-constructs CborCheckpointStoreAdapter (dynamic import, same pattern as patchJournal) when no explicit store is given. Seek cache paths in MaterializeController are left on the legacy codec path — they operate on raw buffers, not blob OIDs, so they don't map to the CheckpointStorePort abstraction. ProvenanceIndex serialization (line 313) deferred to Slice 4.
The serializer files (CheckpointSerializerV5, StateSerializerV5, Frontier) still export legacy serialize/deserialize functions used by callers not yet migrated (MaterializeController, BoundaryTransitionRecord, ComparisonController, QueryController). They keep their defaultCodec fallback until ALL callers are migrated in later slices. CheckpointService is codec-free: it routes through CheckpointStorePort when available (auto-constructed in WarpRuntime.open()). Tripwire: 36/36 (27 patch + 9 checkpoint). 5,264 tests pass. Lint clean.
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
Replaces the
WorldlineSourcediscriminated union (tag switching onkind: 'live'|'coordinate'|'strand') with a proper class hierarchyper the Systems-Style JavaScript manifesto (P1, P2, P3, P7).
Cycle 0005 attempted this and failed — fake class hierarchy, no
validation, kept kind tags, let tsc drive the design. Cycle 0006
(noun audit) mapped the concept against the AION papers and identified
that WorldlineSource is a worldline selector, not a frontier spec.
Cycle 0007 designed the API with theory-correct naming.
What changed
New domain types:
WorldlineSelector— abstract base withfrom()factory andtoDTO()bridgeLiveSelector— canonical worldline (all writers merged)CoordinateSelector— hypothetical worldline at specific writer tips(private
#frontierwith defensive copy getter for real Map immutability)StrandSelector— single writer's isolated worldlineInternal migration:
WorldlineSelector.from().clone()instanceofdispatchPublic API (unchanged):
Worldline.sourceandObserver.sourcestill return plain DTOsvia
toDTO()— no breaking changeWorldlineSourcetype and interfaces unchanged in index.d.tsplain
{ kind }objects (handled byfrom()at boundaries)Design docs
docs/design/0005-close-audit-reds/— failed cycle retrodocs/design/0006-noun-audit/— theory-informed noun mappingdocs/design/0007-viewpoint-design/— full design with paper mappingTest plan
freeze, clone, from, instanceof, toDTO, mutation isolation)
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
Documentation