fix(M12): complete T8 JANK + T9 TSK TSK — 42 STANK items#55
Merged
flyingrobots merged 5 commits intomainfrom Feb 28, 2026
Merged
fix(M12): complete T8 JANK + T9 TSK TSK — 42 STANK items#55flyingrobots merged 5 commits intomainfrom
flyingrobots merged 5 commits intomainfrom
Conversation
T8 remaining (6 items): J3 single rev-parse in readRef, J4 pooled blob reads in readTree, J6 prefix-check findAttachedData, J7 schema version cache, J14 selective checkpoint error handling, J16 tip-only semantics docs. T9 complete (38 items): B67 receipt reverse index, B73 orsetClone, B74 Writer reentrancy docs, B75 VV zero-counter assertion, plus 34 TSK TSK items across JoinReducer, CheckpointService, GitGraphAdapter, CRDT/Utils, Services/Errors, Writer/CLI clusters. Notable behavioral changes: - canonicalStringify now detects circular references (T18) - matchGlob regex cache evicts at 1000 entries (T19) - PatchSession throws WriterError instead of generic Error (T33) - schemas.js rejects Infinity/NaN via Number.isFinite (T28) - IncrementalIndexUpdater caches _nextLabelId for O(1) (T35) - SyncProtocol uses frontierToObject/objectToFrontier helpers (T23) - GitGraphAdapter extracts _createCommit shared helper (T6) All 4425 tests pass. Lint clean. noCoordination gate green.
The field is always present at runtime — createEmptyStateV5() and deserializeFullStateV5() both initialize it to an empty Map. The [optional] marker from T3 was semantically wrong (it's only absent in serialized older checkpoints, not in the runtime type). Fixes 8 pre-existing tsc TS18048 errors in CheckpointSerializerV5.test.js.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughBroad refactor and documentation updates: changelog/roadmap edits, CLI schema finiteness checks, CRDT OR-Set/VersionVector improvements, service-layer caching and removal optimizations, Git adapter and checkpoint error-handling changes, utility fixes (cycle detection, cache eviction), and JSDoc/throw-type clarifications — no public API signature breaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Release Preflight
If you tag this commit as |
….CR-10)
CR-1 (CRITICAL): Reset _nextLabelId on each computeDirtyShards call to
prevent stale label ID collisions when labels object is reloaded.
CR-2 (HIGH): Fix matchGlob cache eviction — insert before evict and use
>= threshold so newly-compiled regex survives the clear.
CR-3 (HIGH): Move PROPS_PREFIX constant below all import statements in
MaterializedViewService.
CR-4: Document error string-contains matches in checkpoint.methods.js.
CR-7: Update ROADMAP critical path diagram (T8/T9 now ✅).
CR-8: Move T15 from Documentation to Refactored in CHANGELOG.
CR-9: Fix orsetJoin comment ("Copy" not "Clone").
CR-10: Clarify VV zero-counter assertion as invariant, not input validation.
CR-12 (return await) reverted — ESLint return-await rule requires it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
ROADMAP.md (1)
229-229: Normalize gate-verification phrasing across sections.Line 229 reads as gate checks already green, while Line 460 says “pending M12 gate verification.” Pick one canonical state to avoid operator confusion.
Also applies to: 460-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` at line 229, The two statements about the M12 gate are inconsistent: update the phrasing for "M12 Gate" and any mentions like "pending M12 gate verification" so both show the same canonical state (either "verified" or "pending verification"); specifically edit the lines that reference "M12 Gate", "WarpGraph.noCoordination.test.js", and "S2/B116" so they consistently say the chosen status and ensure the follow-up note about extracted items (S2/B116 → M13) is retained; also search the document for other "gate-verification" or "M12" mentions and make them match the canonical phrasing.src/domain/services/MaterializedViewService.js (2)
144-151: Minor: comment wording is slightly misleading.The comment mentions "graph has fewer nodes than sample size," but the actual scenario is when the probabilistic sampling (based on
sampleRate) randomly selects zero nodes. This can occur with low sample rates on small graphs due to random chance, not because the graph size is smaller than a target count.Consider rewording for clarity:
Suggested wording
- // When the initial sample is empty (e.g., graph has fewer nodes than - // sample size), we fall back to using all available nodes. This changes + // When the initial sample is empty (e.g., low sample rate on a small graph + // yields no selections by chance), we fall back to selecting one node. This changes // the distribution but is acceptable since the sample is only used for // layout heuristics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/MaterializedViewService.js` around lines 144 - 151, The comment above the fallback that pushes a node into sampled (when sampled.length === 0) is misleading; update it to state that this fallback handles the case where probabilistic sampling (using rng() and sampleRate) accidentally selects zero nodes (e.g., with low sampleRate on small graphs), rather than implying the graph is smaller than a target sample size; locate the block using sampled, allNodes, and rng() in MaterializedViewService.js and replace the wording accordingly to describe the zero-selection-by-chance scenario and that using a single available node is an acceptable heuristic for layout.
21-24: Move constant definition after all imports.The
PROPS_PREFIXconstant is defined in the middle of import statements (between lines 20 and 24). This breaks import grouping conventions and reduces readability. Move the constant declaration after line 25 where all imports have concluded.Suggested reorganization
import IncrementalIndexUpdater from './IncrementalIndexUpdater.js'; - -/** Prefix for property shard paths in the index tree. */ -const PROPS_PREFIX = 'props_'; import { orsetElements, orsetContains } from '../crdt/ORSet.js'; import { decodeEdgeKey } from './KeyCodec.js'; + +/** Prefix for property shard paths in the index tree. */ +const PROPS_PREFIX = 'props_';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/MaterializedViewService.js` around lines 21 - 24, The PROPS_PREFIX constant is declared between import statements which breaks import grouping; move the const PROPS_PREFIX = 'props_'; so that it appears after all import statements (i.e., below the last import in MaterializedViewService.js) to keep imports grouped together and restore readability; ensure any references to PROPS_PREFIX remain unchanged and that the file still exports/uses it where applicable.src/domain/errors/StorageError.js (1)
38-40: Documentation mentionsObject.assignbut code uses spread syntax.The comment states "Context is merged via Object.assign" but the implementation at lines 46-50 uses spread syntax (
{ ...options.context, ... }). While both yield last-writer-wins semantics, the doc should match the actual mechanism.📝 Suggested documentation fix
- * Context is merged via Object.assign — duplicate keys from the second - * argument overwrite the first. Callers should ensure context keys don't - * collide, or use unique prefixes. + * Context is merged via object spread — explicit `operation` and `oid` + * properties overwrite any same-named keys in `options.context`. Callers + * should avoid passing these keys in context, or use unique prefixes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/errors/StorageError.js` around lines 38 - 40, The class StorageError's JSDoc claims "Context is merged via Object.assign" but the constructor actually merges context using spread syntax; update the comment to reflect the implementation (e.g., "Context is merged via object spread (`{ ...a, ...b }`) — duplicate keys from the second argument overwrite the first") or change the constructor in StorageError to use Object.assign to match the doc; reference the StorageError constructor and the context merge expression to locate the code.src/domain/services/SyncProtocol.js (1)
85-110: Consider consolidating frontier conversion utilities withFrontier.js.The file already imports
cloneFrontierandupdateFrontierfrom./Frontier.js, and the relevant snippets suggestFrontier.jshas similar Map/object conversion patterns. MovingfrontierToObjectandobjectToFrontierthere would centralize all frontier utilities in one module.That said, keeping them private here is reasonable if they're only needed in
SyncProtocol.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/SyncProtocol.js` around lines 85 - 110, The frontierToObject and objectToFrontier helpers belong with other frontier utilities in Frontier.js: remove these two functions from SyncProtocol.js, add them to Frontier.js (alongside cloneFrontier and updateFrontier) and export them (e.g., export function frontierToObject(...) and export function objectToFrontier(...)); then import { frontierToObject, objectToFrontier } from './Frontier.js' in SyncProtocol.js and update any local references to use the imported names. Ensure signatures remain the same and update any tests or usages accordingly.src/infrastructure/adapters/GitGraphAdapter.js (1)
547-547: Consider consistent use ofthis._getExitCodethroughout the class.Some call sites use the module-level
getExitCode(lines 547, 638) while others use the instance methodthis._getExitCode(lines 714, 793). For consistent test mockability as documented in the JSDoc at line 601-603, consider using the instance method everywhere.Also applies to: 638-638, 714-714, 793-793
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/GitGraphAdapter.js` at line 547, In GitGraphAdapter.js, replace direct calls to the module-level function getExitCode with the instance method this._getExitCode throughout the class (e.g., where getExitCode(gitErr) is used) so all call sites use this._getExitCode for consistent test mockability; update the call sites inside the class methods to invoke this._getExitCode(gitErr) and ensure the class contains the _getExitCode method (or its binding) so no external module-level references remain.src/domain/crdt/Dot.js (1)
112-116: Documentation improvement looks good; minor wording nit.The added clarification about
lastIndexOf(':')handling is accurate and helpful. One small observation: empty writer IDs don't produce "unexpected results" — they throw an explicit error at line 132. Consider rewording to "empty writer IDs will throw" for precision, though the current wording is acceptable since it warns users about the edge case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/crdt/Dot.js` around lines 112 - 116, Update the documentation comment describing writer ID parsing (the block that explains use of lastIndexOf(':') in Dot.js) to state that empty writer IDs will throw an explicit error rather than producing "unexpected results"; keep the explanation about supporting colons and numeric counters intact and mention that IDs ending with a colon are still problematic, referencing the same parsing behavior in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Around line 467-475: Update the stale M12 gating/status text so it doesn't
contradict completed tasks: find the M12 gate block and the status entries for
T8 (JANK) and T9 (TSK TSK) and make them consistent with the earlier DONE
markers (T8 and T9 are complete per the T1/T2/T3 sections); either mark T8 and
T9 as ✅ (DONE) in the M12 area or remove the "IN PROGRESS / PENDING" labels, and
ensure the M12 gate line text reflects that all dependent tasks are completed
(including T4, T5, T7, T8, T9).
In `@src/domain/services/JoinReducer.js`:
- Around line 329-336: The effectiveness check uses op.observedDots directly
(which may be a one-shot iterable) instead of the normalized Set targetDots;
update the loops that iterate observed dots (the loop that computes `effective`
and the similar loop later around where `dotToElement` is used) to iterate
`targetDots` instead of `op.observedDots`, keeping the existing checks against
`orset.tombstones` and `dotToElement` and using the same helpers
(`buildDotToElement`, `dotToElement`, `orset.tombstones`, `effective`) so the
normalized collection is consumed safely and `redundant` is computed correctly.
In `@src/domain/utils/canonicalStringify.js`:
- Around line 35-53: The code uses a single global `seen` set to detect cycles
which wrongly treats repeated but acyclic shared references as circular; change
the cycle detection to track the current recursion stack (e.g., rename or add a
Set like `stack` or `ancestors`) and use that for cycle checks instead of the
global `seen`. Specifically, in _canonicalStringify (and the map/object handling
paths where `seen.has(value)` is checked and `seen.add(value)` is called),
replace those checks with `stack.has(value)`/`stack.add(value)` and ensure you
`stack.delete(value)` (or pop) after finishing processing each value so only
nodes on the current recursion path count as cycles; keep any global
memoization/seen logic separate if you still need it for other optimizations.
In `@src/domain/warp/checkpoint.methods.js`:
- Around line 247-257: The JSDoc for _hasSchema1Patches is inaccurate: the
implementation treats both explicit schema:1 and missing schema (schema ===
undefined) as v1 history; update the documentation for the _hasSchema1Patches
method to state that it detects writer tips that are either schema:1 or have no
schema field (undefined), so callers understand both cases are treated as v1 by
the function. Ensure the JSDoc block for _hasSchema1Patches mentions this
behavior and that the method remains a heuristic inspecting only the latest tip
per writer.
---
Nitpick comments:
In `@ROADMAP.md`:
- Line 229: The two statements about the M12 gate are inconsistent: update the
phrasing for "M12 Gate" and any mentions like "pending M12 gate verification" so
both show the same canonical state (either "verified" or "pending
verification"); specifically edit the lines that reference "M12 Gate",
"WarpGraph.noCoordination.test.js", and "S2/B116" so they consistently say the
chosen status and ensure the follow-up note about extracted items (S2/B116 →
M13) is retained; also search the document for other "gate-verification" or
"M12" mentions and make them match the canonical phrasing.
In `@src/domain/crdt/Dot.js`:
- Around line 112-116: Update the documentation comment describing writer ID
parsing (the block that explains use of lastIndexOf(':') in Dot.js) to state
that empty writer IDs will throw an explicit error rather than producing
"unexpected results"; keep the explanation about supporting colons and numeric
counters intact and mention that IDs ending with a colon are still problematic,
referencing the same parsing behavior in the file.
In `@src/domain/errors/StorageError.js`:
- Around line 38-40: The class StorageError's JSDoc claims "Context is merged
via Object.assign" but the constructor actually merges context using spread
syntax; update the comment to reflect the implementation (e.g., "Context is
merged via object spread (`{ ...a, ...b }`) — duplicate keys from the second
argument overwrite the first") or change the constructor in StorageError to use
Object.assign to match the doc; reference the StorageError constructor and the
context merge expression to locate the code.
In `@src/domain/services/MaterializedViewService.js`:
- Around line 144-151: The comment above the fallback that pushes a node into
sampled (when sampled.length === 0) is misleading; update it to state that this
fallback handles the case where probabilistic sampling (using rng() and
sampleRate) accidentally selects zero nodes (e.g., with low sampleRate on small
graphs), rather than implying the graph is smaller than a target sample size;
locate the block using sampled, allNodes, and rng() in
MaterializedViewService.js and replace the wording accordingly to describe the
zero-selection-by-chance scenario and that using a single available node is an
acceptable heuristic for layout.
- Around line 21-24: The PROPS_PREFIX constant is declared between import
statements which breaks import grouping; move the const PROPS_PREFIX = 'props_';
so that it appears after all import statements (i.e., below the last import in
MaterializedViewService.js) to keep imports grouped together and restore
readability; ensure any references to PROPS_PREFIX remain unchanged and that the
file still exports/uses it where applicable.
In `@src/domain/services/SyncProtocol.js`:
- Around line 85-110: The frontierToObject and objectToFrontier helpers belong
with other frontier utilities in Frontier.js: remove these two functions from
SyncProtocol.js, add them to Frontier.js (alongside cloneFrontier and
updateFrontier) and export them (e.g., export function frontierToObject(...) and
export function objectToFrontier(...)); then import { frontierToObject,
objectToFrontier } from './Frontier.js' in SyncProtocol.js and update any local
references to use the imported names. Ensure signatures remain the same and
update any tests or usages accordingly.
In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Line 547: In GitGraphAdapter.js, replace direct calls to the module-level
function getExitCode with the instance method this._getExitCode throughout the
class (e.g., where getExitCode(gitErr) is used) so all call sites use
this._getExitCode for consistent test mockability; update the call sites inside
the class methods to invoke this._getExitCode(gitErr) and ensure the class
contains the _getExitCode method (or its binding) so no external module-level
references remain.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
CHANGELOG.mdROADMAP.mdbin/cli/infrastructure.jsbin/cli/schemas.jssrc/domain/crdt/Dot.jssrc/domain/crdt/LWW.jssrc/domain/crdt/ORSet.jssrc/domain/crdt/VersionVector.jssrc/domain/errors/StorageError.jssrc/domain/errors/WriterError.jssrc/domain/services/CheckpointService.jssrc/domain/services/IncrementalIndexUpdater.jssrc/domain/services/JoinReducer.jssrc/domain/services/MaterializedViewService.jssrc/domain/services/PatchBuilderV2.jssrc/domain/services/QueryBuilder.jssrc/domain/services/SyncProtocol.jssrc/domain/utils/EventId.jssrc/domain/utils/LRUCache.jssrc/domain/utils/RefLayout.jssrc/domain/utils/canonicalStringify.jssrc/domain/utils/matchGlob.jssrc/domain/warp/PatchSession.jssrc/domain/warp/Writer.jssrc/domain/warp/checkpoint.methods.jssrc/infrastructure/adapters/GitGraphAdapter.jssrc/infrastructure/codecs/CborCodec.jstest/unit/domain/services/GitGraphAdapter.test.js
Comment on lines
+170
to
+185
| } catch (err) { | ||
| // "Not found" conditions (missing tree entries, missing blobs) are expected | ||
| // when a checkpoint ref exists but the objects have been pruned or are | ||
| // unreachable. In that case, fall back to full replay by returning null. | ||
| // Decode/corruption errors (e.g., CBOR parse failure, schema mismatch) | ||
| // should propagate so callers see the real problem. | ||
| const msg = err instanceof Error ? err.message : ''; | ||
| if ( | ||
| msg.includes('missing') || | ||
| msg.includes('not found') || | ||
| msg.includes('ENOENT') || | ||
| msg.includes('non-empty string') | ||
| ) { | ||
| return null; | ||
| } | ||
| throw err; |
There was a problem hiding this comment.
Avoid broad message matching that can hide checkpoint corruption.
On Line 178 and Line 181, substring checks are too broad. Errors like “missing required field …” can be misclassified as “not found,” causing _loadLatestCheckpoint() to return null instead of surfacing real corruption.
Proposed tightening (prefer explicit code + scoped message pattern)
- const msg = err instanceof Error ? err.message : '';
- if (
- msg.includes('missing') ||
- msg.includes('not found') ||
- msg.includes('ENOENT') ||
- msg.includes('non-empty string')
- ) {
+ const code = /** `@type` {{code?: string}|null} */ (
+ err && typeof err === 'object' ? err : null
+ )?.code;
+ const msg = err instanceof Error ? err.message.toLowerCase() : '';
+ const isMissingObjectError =
+ code === 'ENOENT' ||
+ /\b(blob|tree|object|checkpoint|ref)\b.*\b(not found|missing)\b/.test(msg);
+ if (isMissingObjectError) {
return null;
}
Member
Author
There was a problem hiding this comment.
Respectfully flagging this as a non-issue. The four substring checks match specific persistence-layer error sources (documented in commit 9e48052):
"missing"— git cat-file on pruned/unreachable objects"not found"— readTree entry lookup failures"ENOENT"— filesystem-level missing path"non-empty string"— readRef/getNodeInfo with empty SHA
CBOR decode errors use messages like "Unexpected end of input" / "Invalid CBOR data" which don't contain any of these substrings. Schema errors don't either. The regex approach would be over-engineering for this codebase — the persistence adapter is internal and its error strings are stable. Inline comments documenting each pattern (added in 9e48052) provide sufficient safety.
There was a problem hiding this comment.
@flyingrobots, thank you for the detailed clarification. You're right—your explanation makes sense. The git cat-file documentation confirms that it outputs <object> SP missing LF for missing objects, which aligns with your substring check. Given that:
- Each substring maps to a documented persistence-layer error source
- CBOR/schema errors use distinct patterns that won't match
- The persistence adapter is internal with stable error strings
- The patterns are already documented in commit 9e48052
...the current approach is appropriate for this codebase. I appreciate you taking the time to explain the rationale.
🐰✨
Noted! Thanks for
teaching me about
your error patterns
(___/)
(=^.^=)
(")_(")
✏️ Learnings added
Learnt from: flyingrobots
Repo: git-stunts/git-warp PR: 55
File: src/domain/warp/checkpoint.methods.js:170-191
Timestamp: 2026-02-28T10:10:38.112Z
Learning: In `src/domain/warp/checkpoint.methods.js`, the `_loadLatestCheckpoint()` function uses four specific substring checks to distinguish "not found" errors from decode/corruption errors:
- `"missing"` — matches git cat-file errors on pruned/unreachable objects
- `"not found"` — matches readTree entry lookup failures
- `"ENOENT"` — matches filesystem-level missing path errors
- `"non-empty string"` — matches readRef/getNodeInfo validation errors with empty SHA
These patterns are documented in commit 9e48052 and are appropriate because the persistence adapter is internal with stable error strings.
Learnt from: flyingrobots
Repo: git-stunts/git-warp PR: 55
File: src/domain/warp/checkpoint.methods.js:170-191
Timestamp: 2026-02-28T10:10:38.112Z
Learning: CBOR decode errors in this codebase use patterns like "Unexpected end of input" or "Invalid CBOR data" that do not contain substrings like "missing", "not found", "ENOENT", or "non-empty string", so they will correctly propagate as corruption errors rather than being treated as missing-object conditions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
ROADMAP.md (1)
613-613:⚠️ Potential issue | 🟡 MinorUpdate stale M12 status sentence in Final Command.
Line 613 still says T8 has “6 remaining” and T9 is still pending, which conflicts with the now-✅ status in the milestone and critical-path sections. Please align this sentence with the current “M12 complete / S2 deferred to M13” state.
Suggested doc fix
-M12 is top priority — T8 (6 remaining JANK items) and T9 (TSK TSK batch) are the last tasks. Edge property encoding (B116/S2) was extracted to M13 as a dedicated schema migration milestone requiring its own design phase. M13 must land before new features (M11). +M12 is complete (including T8/T9). Edge property encoding (B116/S2) was extracted to M13 as a dedicated schema migration milestone requiring its own design phase. M13 must land before new features (M11).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` at line 613, Update the Final Command sentence in ROADMAP.md to reflect that M12 is complete and S2 (edge property encoding) was deferred to M13: find the sentence mentioning "M12 is top priority — T8 (6 remaining JANK items) and T9 (TSK TSK batch) are the last tasks" and replace it with a concise status such as "M12 complete; T8 and T9 are done and S2 (edge property encoding) has been deferred to M13, which will include its own design phase and must land before new features (M11)." Ensure references to M12, T8, T9, S2, and M13 are updated consistently with the milestone and critical-path sections.src/domain/warp/checkpoint.methods.js (2)
262-262:⚠️ Potential issue | 🟡 MinorAlign
_hasSchema1Patchesreturn docs with actual v1 detection behavior.The return description still says schema:1 only, but implementation also treats missing schema (
undefined) as legacy v1.Doc fix
- * `@returns` {Promise<boolean>} True if any writer tip is schema:1 + * `@returns` {Promise<boolean>} True if any writer tip is schema:1 (or omits `schema`, treated as legacy v1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/warp/checkpoint.methods.js` at line 262, Update the JSDoc for _hasSchema1Patches to reflect the actual detection logic: change the `@returns` description to state it returns true if any writer tip is schema:1 or has no schema (legacy v1), rather than saying "schema:1 only"; keep the implementation unchanged and reference the _hasSchema1Patches function name so reviewers can find the doc update easily.
170-191:⚠️ Potential issue | 🟠 MajorNarrow the “not found” fallback classification in checkpoint load path.
The current substring checks are still broad and can swallow corruption/decode errors by returning
nullon generic"missing"text. Restrict fallback to explicit missing-object/ref patterns (andENOENT) and rethrow the rest.Proposed tightening
- const msg = err instanceof Error ? err.message : ''; - if ( - msg.includes('missing') || - msg.includes('not found') || - msg.includes('ENOENT') || - msg.includes('non-empty string') - ) { + const code = /** `@type` {{code?: string}|null} */ ( + err && typeof err === 'object' ? err : null + )?.code; + const msg = err instanceof Error ? err.message.toLowerCase() : ''; + const isMissingObjectError = + code === 'ENOENT' || + /\b(blob|tree|object|checkpoint|ref)\b.*\b(not found|missing)\b/.test(msg) || + msg.includes('non-empty string'); + if (isMissingObjectError) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/warp/checkpoint.methods.js` around lines 170 - 191, The catch block that inspects err.message (the const msg variable) is too broad because msg.includes('missing') can swallow decode/corruption errors; update the condition in the catch (located where checkpoint load falls back to null) to only treat explicit missing-object/ref patterns and ENOENT as recoverable: replace the generic msg.includes('missing') test with checks for precise tokens like 'missing object', 'object missing', 'missing blob', and keep msg.includes('not found') only if you also verify it refers to a ref/tree lookup (e.g., match /\bnot found\b/ or 'readTree: not found'), and preserve msg.includes('ENOENT'); otherwise rethrow the error. Ensure you modify the conditional that currently returns null so only those specific patterns trigger the null fallback and all other errors are thrown.
🧹 Nitpick comments (2)
CHANGELOG.md (2)
27-27: Minor grammar refinement for clarity.The phrase "O(1) per new label" is slightly awkward. Consider:
- "cached
_nextLabelIdfor O(1) label creation" or- "cached
_nextLabelIdto achieve O(1) cost per new label"This makes the performance improvement clearer to readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 27, The changelog line is slightly unclear; update the phrasing referencing IncrementalIndexUpdater and the cached _nextLabelId to clarify the performance improvement—for example change "cached `_nextLabelId` for O(1) per new label" to "cached `_nextLabelId` for O(1) label creation" or "cached `_nextLabelId` to achieve O(1) cost per new label" so readers clearly understand the O(1) improvement for creating new labels.
34-34: Consider formatting the long service list for readability.The parenthetical list of 20+ services spans a very long line, which may be difficult to read. Consider one of these alternatives:
- Summarize: "JSDoc/comment additions across CheckpointService, GitGraphAdapter, CRDT modules, and 10+ other services."
- Format as sub-list (if detail is important):
- **TSK TSK documentation batch** — 20+ JSDoc/comment additions: - CheckpointService (T4, T25, T26) - GitGraphAdapter (T5, T7) - CborCodec (T8), LWW (T10), EventId (T12) - [additional services]- Link to commit/PR: "JSDoc/comment additions across 16 services (see T3-T38 tasks in PR
#55)."The current format makes it hard to scan and may overwhelm readers who just want a high-level understanding of changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 34, The long parenthetical services list in the "TSK TSK documentation batch" changelog entry is hard to read; update the CHANGELOG.md entry for "TSK TSK documentation batch" to either (a) summarize the affected modules (e.g., "JSDoc/comment additions across CheckpointService, GitGraphAdapter, CRDT modules, and 10+ other services") or (b) expand into a short sub-list under that entry that breaks out the key items (e.g., CheckpointService (T4, T25, T26), GitGraphAdapter (T5, T7), CborCodec (T8), LWW (T10), EventId (T12), VersionVector (T38), LRUCache (T20), RefLayout (T31), QueryBuilder (T13), MaterializedViewService (T14, T34), WriterError (T16), StorageError (T17, T29), SyncProtocol (T24), JoinReducer (T3), IncrementalIndexUpdater (T36)) so the line wraps and is easy to scan; keep the heading "TSK TSK documentation batch" and the total count ("20+") intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Line 460: The ROADMAP has conflicting gate-state wording for M12 SCALPEL:
update the two occurrences referencing M12 SCALPEL (the line containing "pending
M12 gate verification" and the earlier line that states the gate criteria as
green) so they use one canonical state; decide whether M12 is "gate verified
(green)" or still "pending verification" and then change the text for the M12
SCALPEL entry and the gate-criteria summary to match exactly (same wording) to
avoid operator confusion.
---
Duplicate comments:
In `@ROADMAP.md`:
- Line 613: Update the Final Command sentence in ROADMAP.md to reflect that M12
is complete and S2 (edge property encoding) was deferred to M13: find the
sentence mentioning "M12 is top priority — T8 (6 remaining JANK items) and T9
(TSK TSK batch) are the last tasks" and replace it with a concise status such as
"M12 complete; T8 and T9 are done and S2 (edge property encoding) has been
deferred to M13, which will include its own design phase and must land before
new features (M11)." Ensure references to M12, T8, T9, S2, and M13 are updated
consistently with the milestone and critical-path sections.
In `@src/domain/warp/checkpoint.methods.js`:
- Line 262: Update the JSDoc for _hasSchema1Patches to reflect the actual
detection logic: change the `@returns` description to state it returns true if any
writer tip is schema:1 or has no schema (legacy v1), rather than saying
"schema:1 only"; keep the implementation unchanged and reference the
_hasSchema1Patches function name so reviewers can find the doc update easily.
- Around line 170-191: The catch block that inspects err.message (the const msg
variable) is too broad because msg.includes('missing') can swallow
decode/corruption errors; update the condition in the catch (located where
checkpoint load falls back to null) to only treat explicit missing-object/ref
patterns and ENOENT as recoverable: replace the generic msg.includes('missing')
test with checks for precise tokens like 'missing object', 'object missing',
'missing blob', and keep msg.includes('not found') only if you also verify it
refers to a ref/tree lookup (e.g., match /\bnot found\b/ or 'readTree: not
found'), and preserve msg.includes('ENOENT'); otherwise rethrow the error.
Ensure you modify the conditional that currently returns null so only those
specific patterns trigger the null fallback and all other errors are thrown.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 27: The changelog line is slightly unclear; update the phrasing
referencing IncrementalIndexUpdater and the cached _nextLabelId to clarify the
performance improvement—for example change "cached `_nextLabelId` for O(1) per
new label" to "cached `_nextLabelId` for O(1) label creation" or "cached
`_nextLabelId` to achieve O(1) cost per new label" so readers clearly understand
the O(1) improvement for creating new labels.
- Line 34: The long parenthetical services list in the "TSK TSK documentation
batch" changelog entry is hard to read; update the CHANGELOG.md entry for "TSK
TSK documentation batch" to either (a) summarize the affected modules (e.g.,
"JSDoc/comment additions across CheckpointService, GitGraphAdapter, CRDT
modules, and 10+ other services") or (b) expand into a short sub-list under that
entry that breaks out the key items (e.g., CheckpointService (T4, T25, T26),
GitGraphAdapter (T5, T7), CborCodec (T8), LWW (T10), EventId (T12),
VersionVector (T38), LRUCache (T20), RefLayout (T31), QueryBuilder (T13),
MaterializedViewService (T14, T34), WriterError (T16), StorageError (T17, T29),
SyncProtocol (T24), JoinReducer (T3), IncrementalIndexUpdater (T36)) so the line
wraps and is easy to scan; keep the heading "TSK TSK documentation batch" and
the total count ("20+") intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.mdROADMAP.mdsrc/domain/crdt/ORSet.jssrc/domain/crdt/VersionVector.jssrc/domain/services/IncrementalIndexUpdater.jssrc/domain/services/MaterializedViewService.jssrc/domain/utils/matchGlob.jssrc/domain/warp/checkpoint.methods.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/domain/utils/matchGlob.js
- src/domain/crdt/VersionVector.js
CR-R1 (MAJOR): canonicalStringify shared-reference false positive — cycle detection now uses stack-based tracking (try/finally delete) instead of ever-growing seen set. Valid DAG structures with shared references no longer throw. Added 5 regression tests (circular + shared + diamond). CR-R2 (MINOR): nodeRemoveOutcome/edgeRemoveOutcome now iterate the normalized targetDots Set instead of raw op.observedDots for consistency with the reverse-index lookup. CR-R3 (MINOR): _hasSchema1Patches @returns JSDoc now documents that schema === undefined is also treated as legacy v1. CR-R4 (DOC): ROADMAP.md Final Command text updated — M12 complete, gate verified. Removed stale "T8 (6 remaining)" / "T9 pending" text. CR-R5 (DOC): ROADMAP.md gate-verification wording aligned — "pending M12 gate verification" → "gate verified".
Satisfies strict TypeScript checking for self-referential test objects.
Member
Author
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
21-23: Consider consolidating repeated entries for the same subsystems.
canonicalStringifyandmatchGlobappear in both Fixed and Refactored. That’s valid, but combining/aliasing them once (with follow-up notes) would make scanning easier for readers.Also applies to: 40-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 21 - 23, Consolidate duplicate changelog entries for the same subsystems by merging the repeated notes for canonicalStringify and matchGlob into a single entry each (retain their identifiers T18 and T19) and adding a short parenthetical or bullet sub-note indicating which categories they touch (e.g., "Fixed" and "Refactored"); do the same for commitNode/commitNodeWithTree (T6) while keeping the mention of the extracted helper _createCommit, and update the other duplicate region (lines referenced as 40-43) similarly so each subsystem appears once with cross-reference notes rather than duplicated full entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 21-23: Consolidate duplicate changelog entries for the same
subsystems by merging the repeated notes for canonicalStringify and matchGlob
into a single entry each (retain their identifiers T18 and T19) and adding a
short parenthetical or bullet sub-note indicating which categories they touch
(e.g., "Fixed" and "Refactored"); do the same for commitNode/commitNodeWithTree
(T6) while keeping the mention of the extracted helper _createCommit, and update
the other duplicate region (lines referenced as 40-43) similarly so each
subsystem appears once with cross-reference notes rather than duplicated full
entries.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdROADMAP.mdsrc/domain/services/JoinReducer.jssrc/domain/utils/canonicalStringify.jssrc/domain/warp/checkpoint.methods.jstest/unit/domain/utils/canonicalStringify.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/domain/warp/checkpoint.methods.js
- src/domain/utils/canonicalStringify.js
flyingrobots
added a commit
that referenced
this pull request
Feb 28, 2026
- Persistence error code enum (replace string matching) - Circular/shared-reference test helpers - CHANGELOG follow-up sub-section convention - Pre-push hook performance (tiered smoke tests)
5 tasks
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
edgeBirthEventcorrected from optional to required inWarpStateV5typedef — it's always present at runtime (resolves 8 pre-existing TS18048 errors)Notable behavioral changes
canonicalStringifydetects circular references via WeakSet (T18)matchGlobregex cache evicts at 1000 entries (T19)PatchSessionthrowsWriterErrorinstead of genericError(T33)schemas.jsrejects Infinity/NaN viaNumber.isFiniterefinement (T28)IncrementalIndexUpdatercaches_nextLabelIdfor O(1) per new label (T35)SyncProtocolfrontier conversions extracted tofrontierToObject/objectToFrontierhelpers (T23)GitGraphAdapterextracts_createCommitshared helper, eliminates 70% duplication (T6)readRefsingle rev-parse replacing two-call pattern (J3)readTreepooled concurrent blob reads, batch size 16 (J4)M12 SCALPEL status after this PR
All tasks complete: T1 ✅ T2 ✅ T3 ✅ T4 ✅ T5 ✅ T7 ✅ T8 ✅ T9 ✅. T6 (EdgePropSet) extracted to dedicated M13 milestone.
Test plan
WarpGraph.noCoordination.test.js— 7/7 greenSummary by CodeRabbit
Bug Fixes
Performance
Documentation
Refactor
Tests