Open
Conversation
Invariant: every source file in src/ must have 100% line coverage. Baseline: 92.12% lines (with barrel file exclusions). Ratchet: vitest coverage thresholds with autoUpdate — coverage can only go up. @vitest/coverage-v8 added as devDependency. Design doc: hill is 100% line coverage. Strategy: controllers first (smallest LOC), then strand services, then WarpApp/Core/Runtime. Cycle 0010 — 100-percent-coverage
…oller, SubscriptionController, ForkController, CheckpointController StrandController (17 tests): pure delegation verification for all 14 methods ProvenanceController (24 tests): backward cone BFS, causal sort, degraded/null guards SubscriptionController (53 tests): subscribe/watch/notify with glob filtering, polling, replay ForkController (38 tests): fork validation, ancestor walking, checkpoint relation, rollback CheckpointController (31 tests): checkpoint lifecycle, GC, migration boundary, coverage sync 5569 total tests passing. 1,398 LOC of previously untested code now covered. Cycle 0010 — 100-percent-coverage
PatchController (62 tests): createPatch auto-materialize, patch/patchMany, _onPatchCommitted eager apply, chain walking, encrypted blob reads, discoverWriters/Ticks, join CRDT merge, writer() delegation SyncController (73 tests): frontier, status, sync request/response, trust gate enforcement, direct peer sync, HTTP sync with retry/abort/auth, serve port validation QueryController (67 tests): state queries, neighbor lookups (linear + indexed), content attachments (node + edge), query/observer/worldline creation, translationCost 5771 total tests passing. 8/10 controllers now covered. Cycle 0010 — 100-percent-coverage
Covers buildPatchDivergence, compareCoordinates, compareStrand, planCoordinateTransfer, planStrandTransfer, selector normalization, strand resolution, state hash fallback, ceiling filtering, and edge cases including multi-writer frontiers and scope passthrough.
MaterializeController (62 tests): full/incremental materialize, ceiling time-travel, receipt collection, auto-checkpoint, subscriber notify, view build/invalidate, coordinate materialize, seek cache, index verify ComparisonController (61 tests): patch divergence, coordinate/strand comparison, transfer planning, selector normalization, state hash, ceiling filtering All 10 controllers now have unit tests. 488 new controller tests total. 5894 tests passing across 338 files. Cycle 0010 — 100-percent-coverage
📝 WalkthroughWalkthroughAdds a full-coverage design/invariant, CI/test tooling for V8 coverage, a coverage-ratchet script gate, many new Vitest unit test suites across controllers/services/graph/runtime, and numerous backlog/method docs and agent/process guidance. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)test/unit/domain/WarpGraph.test.js[] test/unit/domain/services/strand/ConflictAnalyzerService.test.jsThanks 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 |
Coverage threshold bumped by vitest autoUpdate after 488 new controller tests. Cycle 0010 — 100-percent-coverage
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/services/controllers/CheckpointController.test.js`:
- Around line 91-97: The JSDoc `@returns` annotation on createMockHost forces
ReturnType<typeof createMockHost> to an index-signature-only Record and erases
the concrete object shape; remove the explicit `@returns {Record<string,
unknown>}` line from the JSDoc for createMockHost so TypeScript can infer the
actual return object shape (so uses of ReturnType<typeof createMockHost> and
downstream property accesses like host._persistence, host.discoverWriters,
host._cachedState are typed correctly).
In `@test/unit/domain/services/controllers/MaterializeController.test.js`:
- Around line 491-495: The test calls MaterializeController._resolveCeiling()
with no argument but the implementation checks for the 'ceiling' key in the
options object; update the spec to call ctrl._resolveCeiling({}) so the method
can check options and then fall back to the instance value (host._seekCeiling =
null) as intended; change the test invocation in the it block to pass an empty
options object to cover the “ceiling key omitted” branch.
🪄 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: 4d276df9-bd61-4844-b56f-39f16ce422a7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
docs/design/0010-100-percent-coverage/100-percent-coverage.mddocs/invariants/full-coverage.mddocs/method/backlog/asap/DX_timeoutms-missing-from-type-surface.mdpackage.jsontest/unit/domain/services/controllers/CheckpointController.test.jstest/unit/domain/services/controllers/ComparisonController.test.jstest/unit/domain/services/controllers/ForkController.test.jstest/unit/domain/services/controllers/MaterializeController.test.jstest/unit/domain/services/controllers/PatchController.test.jstest/unit/domain/services/controllers/ProvenanceController.test.jstest/unit/domain/services/controllers/QueryController.test.jstest/unit/domain/services/controllers/StrandController.test.jstest/unit/domain/services/controllers/SubscriptionController.test.jstest/unit/domain/services/controllers/SyncController.test.jsvitest.config.js
Comment on lines
+91
to
+97
| /** | ||
| * Builds a mock host with sensible defaults. | ||
| * | ||
| * @param {Record<string, unknown>} [overrides] | ||
| * @returns {Record<string, unknown>} | ||
| */ | ||
| function createMockHost(overrides = {}) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
sed -n '91,140p' test/unit/domain/services/controllers/CheckpointController.test.js
printf '\n---\n'
rg -n "Record<string, unknown>|ReturnType<typeof createMockHost>|host\.(discoverWriters|materialize|_persistence|_cachedState|_stateDirty)" test/unit/domain/services/controllers/CheckpointController.test.jsRepository: git-stunts/git-warp
Length of output: 4920
Remove the overly broad return type annotation from createMockHost().
The explicit @returns {Record<string, unknown>} JSDoc annotation forces ReturnType<typeof createMockHost> to resolve to an index-signature-only type, erasing the concrete object shape. This causes TypeScript TS4111 errors on all property accesses downstream (e.g., host._persistence, host.discoverWriters, host._cachedState). Delete the return annotation to restore type inference from the return statement's concrete object literal.
Minimal fix
/**
* Builds a mock host with sensible defaults.
*
* `@param` {Record<string, unknown>} [overrides]
- * `@returns` {Record<string, unknown>}
*/
function createMockHost(overrides = {}) {📝 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
| /** | |
| * Builds a mock host with sensible defaults. | |
| * | |
| * @param {Record<string, unknown>} [overrides] | |
| * @returns {Record<string, unknown>} | |
| */ | |
| function createMockHost(overrides = {}) { | |
| /** | |
| * Builds a mock host with sensible defaults. | |
| * | |
| * `@param` {Record<string, unknown>} [overrides] | |
| */ | |
| function createMockHost(overrides = {}) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/services/controllers/CheckpointController.test.js` around
lines 91 - 97, The JSDoc `@returns` annotation on createMockHost forces
ReturnType<typeof createMockHost> to an index-signature-only Record and erases
the concrete object shape; remove the explicit `@returns {Record<string,
unknown>}` line from the JSDoc for createMockHost so TypeScript can infer the
actual return object shape (so uses of ReturnType<typeof createMockHost> and
downstream property accesses like host._persistence, host.discoverWriters,
host._cachedState are typed correctly).
Comment on lines
+491
to
+495
| it('returns null when no options and no instance ceiling', () => { | ||
| const { ctrl, host } = setup(); | ||
| host._seekCeiling = null; | ||
| expect(ctrl._resolveCeiling()).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Pass an options object to _resolveCeiling() in this spec.
The current MaterializeController._resolveCeiling() implementation checks 'ceiling' in options, so calling it with no argument throws before it can fall back to the instance ceiling. Use {} here if the goal is to cover the “ceiling key omitted” branch.
🧪 Minimal fix
- expect(ctrl._resolveCeiling()).toBeNull();
+ expect(ctrl._resolveCeiling({})).toBeNull();📝 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('returns null when no options and no instance ceiling', () => { | |
| const { ctrl, host } = setup(); | |
| host._seekCeiling = null; | |
| expect(ctrl._resolveCeiling()).toBeNull(); | |
| }); | |
| it('returns null when no options and no instance ceiling', () => { | |
| const { ctrl, host } = setup(); | |
| host._seekCeiling = null; | |
| expect(ctrl._resolveCeiling({})).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/services/controllers/MaterializeController.test.js` around
lines 491 - 495, The test calls MaterializeController._resolveCeiling() with no
argument but the implementation checks for the 'ceiling' key in the options
object; update the spec to call ctrl._resolveCeiling({}) so the method can check
options and then fall back to the instance value (host._seekCeiling = null) as
intended; change the test invocation in the it block to pass an empty options
object to cover the “ceiling key omitted” branch.
Covers all 12 public methods, key private methods, and the footprint overlap classification algorithm. Tests verify: - Strand lifecycle (create, get, list, drop, getOrThrow) - Braid coordination with base observation matching - Materialization via JoinReducer with receipt collection - PatchBuilder wiring and reentrancy guard - Intent queuing, listing, and tick processing - Patch collection (base + overlay + braided, deduplication) - Entity-level provenance queries (patchesFor) - Commit pipeline (journal, codec fallback, blob storage) - Braid ref synchronization (create, update, stale cleanup) - All error paths (10 distinct StrandError codes)
Covers the full public API (analyze) and _hash caching: - Empty/trivial cases (no writers, no patches, single writer) - Supersession detection (LWW winner identification) - Redundancy detection (identical property writes) - Eventual override detection (NodeAdd + PropSet patterns) - Option filtering (kind, writerId, entityId, target selector) - Lamport ceiling and scan budget application - Evidence levels (summary, standard, full) - Validation errors (14 rejection cases for malformed options) - Resolved coordinate structure (frontier kind, digest, ceiling) - Snapshot hash determinism (identical inputs → identical hash) - Complex multi-writer scenarios (3 writers, edge ops, causal ordering) - Truncation diagnostics for budget-exceeded scans - Trace structure validation (all required fields on traces and losers)
Phase 4 — cover the orchestration layer: WarpApp (29 tests): - All delegation paths: writer, createPatch, patch, patchMany - Content methods (8): getContent, getContentStream, getContentOid, getContentMeta + edge variants via callInternalRuntimeMethod - Strand delegation (10): create, get, list, braid, drop, createStrandPatch, patchStrand, queueStrandIntent, listStrandIntents, tickStrand - Observer overloads and query delegation WarpCore (16 tests): - _adopt: early return for existing WarpCore + prototype mutation path - Content methods (8) with prototype chain resolution through callInternalRuntimeMethod → _queryController - Effect pipeline accessors with null pipeline WarpRuntime (8 tests): - _extractTrustedWriters: trusted/untrusted/empty assessment filtering - _maxLamportFromState: empty/single/multi-writer frontiers - temporal.eventually/always: exercises loadAllPatches callback
Add focused tests for streamUtils, PatchSession, missing port stubs, and PatchJournalPort coverage gaps. Refresh the vitest lines threshold to the passing baseline and capture the WarpRuntime.open options-class follow-up in METHOD backlog.
Add focused tests for error/type utility modules, trust verdict derivation, internal runtime method lookup, default codec and crypto fallback paths, glob matching, and small state/stream branches. Refresh the vitest line threshold to the new passing baseline.
flyingrobots
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
test/unit/domain/services/VisibleStateTransferPlannerV5.test.js (1)
196-213: Use two equivalent readers (not the same instance) for idempotency.Using one
readerfor both source/target can mask identity/shared-reference bugs. Prefer separate reader instances built from identical fixtures.Suggested adjustment
- const reader = createReader({ - nodes: ['alpha'], - edges: [{ from: 'alpha', to: 'alpha', label: 'self' }], - nodeProps: { alpha: { status: 'ready' } }, - edgeProps: { [makeEdgeKey('alpha', 'alpha', 'self')]: { weight: 1 } }, - nodeContentMeta: { alpha: { oid: 'same-node', mime: 'text/plain', size: 4 } }, - edgeContentMeta: { [makeEdgeKey('alpha', 'alpha', 'self')]: { oid: 'same-edge', mime: 'text/plain', size: 4 } }, - }); + const makeState = () => ({ + nodes: ['alpha'], + edges: [{ from: 'alpha', to: 'alpha', label: 'self' }], + nodeProps: { alpha: { status: 'ready' } }, + edgeProps: { [makeEdgeKey('alpha', 'alpha', 'self')]: { weight: 1 } }, + nodeContentMeta: { alpha: { oid: 'same-node', mime: 'text/plain', size: 4 } }, + edgeContentMeta: { [makeEdgeKey('alpha', 'alpha', 'self')]: { oid: 'same-edge', mime: 'text/plain', size: 4 } }, + }); + const sourceReader = createReader(makeState()); + const targetReader = createReader(makeState()); @@ - const plan = await planVisibleStateTransferV5(reader, reader, { + const plan = await planVisibleStateTransferV5(sourceReader, targetReader, { loadNodeContent, loadEdgeContent, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/VisibleStateTransferPlannerV5.test.js` around lines 196 - 213, The test currently uses the same reader instance for both source and target which can hide shared-reference bugs; create two separate but equivalent readers (e.g. sourceReader and targetReader) by calling createReader twice with the same fixture data and use those when calling planVisibleStateTransferV5 (keep the loadNodeContent and loadEdgeContent mocks unchanged) so the test asserts idempotency using distinct instances.test/unit/domain/services/AuditReceiptService.test.js (1)
269-273:Number.MAX_SAFE_INTEGER + 1loses precision and equalsNumber.MAX_SAFE_INTEGER.Due to IEEE 754 floating-point representation,
Number.MAX_SAFE_INTEGER + 1(9007199254740992) is representable, but adding 1 again would lose precision. However, the real issue is thatNumber.MAX_SAFE_INTEGER + 1is technically still a valid integer (it's2^53). If the implementation checks> Number.MAX_SAFE_INTEGER, this test passes, but if it checks>= Number.MAX_SAFE_INTEGER + 1or usesNumber.isSafeInteger(), the behavior differs.Consider using a clearly unsafe value like
Number.MAX_SAFE_INTEGER + 100or2 ** 53 + 1(which loses precision) to make the test intent clearer and more robust.💡 Suggested improvement
it('rejects timestamps above Number.MAX_SAFE_INTEGER', () => { const f = validFields(); - f.timestamp = Number.MAX_SAFE_INTEGER + 1; + f.timestamp = Number.MAX_SAFE_INTEGER + 1000; expect(() => buildReceiptRecord(f)).toThrow('exceeds Number.MAX_SAFE_INTEGER'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/AuditReceiptService.test.js` around lines 269 - 273, The test uses Number.MAX_SAFE_INTEGER + 1 which can be ambiguous due to floating-point precision; update the failing timestamp in the test (set on f.timestamp in AuditReceiptService.test.js created via validFields()) to a clearly unsafe value such as 2 ** 53 + 1 or Number.MAX_SAFE_INTEGER + 100 so buildReceiptRecord(f) deterministically rejects it; adjust the expect call to remain the same but assign f.timestamp = 2 ** 53 + 1 (or +100) before calling buildReceiptRecord.test/unit/domain/services/IncrementalIndexUpdater.test.js (1)
732-882: Consider extracting a small guard-test harness to reduce repetition.This block repeats the same setup patterns many times (
state/tree/updater/caches), which makes future guard-path additions noisier than needed.♻️ Optional refactor sketch
+function createGuardHarness() { + const state = buildState({ nodes: ['A'], edges: [], props: [] }); + const tree = buildTree(state); + const updater = /** `@type` {any} */ (new IncrementalIndexUpdater()); + const loadShard = (path) => tree[path]; + return { state, tree, updater, loadShard }; +} + +function emptyDiff() { + return { + nodesAdded: [], + nodesRemoved: [], + edgesAdded: [], + edgesRemoved: [], + propsChanged: [], + }; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/IncrementalIndexUpdater.test.js` around lines 732 - 882, The tests repeat creating state/tree/updater and various caches; extract a small test harness helper (e.g., buildGuardHarness) that constructs and returns the common fixtures (state, tree, updater, metaCache, fwdCache, revCache, out, cache) and use it in tests that exercise _handleNodeRemove, _purgeNodeEdges, _handleEdgeAdd, _handleEdgeRemove, _flushEdgeShards, _loadLabels, _getOrBuildAliveEdgeAdjacency and _removeEdgeKeyFromAdjacency to eliminate duplication and make each test body only set up the minimal per-test inputs and assertions.test/unit/domain/services/VisibleStateScopeV1.test.js (2)
115-124: Rename test to match what it actually asserts.Line 115 mentions edge filtering, but this case only validates node-level inclusion/exclusion. Renaming the test title will reduce ambiguity during failures.
✏️ Suggested rename
- it('matches include-empty rules and filters edges by endpoint visibility', () => { + it('matches include-empty rules for node visibility', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/VisibleStateScopeV1.test.js` around lines 115 - 124, The test title is misleading — update the 'it' description in the test containing normalizeVisibleStateScopeV1 and nodeIdInVisibleStateScope to reflect that it verifies node-level inclusion/exclusion (not edge filtering); change the string from something like "matches include-empty rules and filters edges by endpoint visibility" to a clearer title such as "matches include-empty rules and excludes node IDs by prefix" so failures correctly indicate the asserted behavior.
149-155: Consider order-insensitive assertion for node IDs if ordering is non-contractual.If
getNodes()ordering is not guaranteed bycreateStateReaderV5, this exact-array assertion can become flaky.🧪 Optional hardening
- expect(reader.getNodes()).toEqual(['comparison-artifact:cmp-1', 'task:1']); + expect(new Set(reader.getNodes())).toEqual( + new Set(['comparison-artifact:cmp-1', 'task:1']), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/VisibleStateScopeV1.test.js` around lines 149 - 155, The test assumes a stable ordering from createStateReaderV5 by asserting reader.getNodes() equals an exact array, which can be flaky; change the assertion for getNodes() in VisibleStateScopeV1.test.js to be order-insensitive (e.g., compare sorted arrays or use a set/array-contains-same-members style assertion) while keeping the existing getEdges() check if edge order is contractual; update the test line that calls reader.getNodes() to sort both actual and expected node lists or use expect.arrayContaining/containSameMembers equivalent so tests no longer fail due to non-deterministic node ordering.test/unit/domain/services/Observer.test.js (1)
640-640: Avoid order-coupled assertion forgetNodes()unless order is contractually guaranteed.Line 640 currently hard-codes output order, which can make this test flaky if implementation changes ordering without changing semantics.
Suggested assertion shape
- expect(await view.getNodes()).toEqual(['user:bob', 'user:alice']); + const nodes = await view.getNodes(); + expect(nodes).toHaveLength(2); + expect(nodes).toEqual(expect.arrayContaining(['user:alice', 'user:bob']));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/Observer.test.js` at line 640, Test currently asserts exact order from view.getNodes() which is not guaranteed; change the assertion in the Observer test to be order-insensitive by verifying the returned array contains the expected items regardless of order (for example use an unordered matcher like expect.arrayContaining([...]) combined with length check, or sort both actual and expected before comparing) and update the assertion referencing view.getNodes() and the expected ['user:bob','user:alice'] accordingly.test/unit/domain/services/JoinReducer.test.js (1)
595-617: UselwwValue(...)instead of reaching into LWW internals.On Line 616, asserting
?.valuecouples the test to internal entry shape. PreferlwwValue(...)for a stable contract-level assertion.Suggested test assertion update
- expect(cloned.prop.get(encodePropKey('x', 'name'))?.value).toEqual(createInlineValue('Alice')); + expect(lwwValue(cloned.prop.get(encodePropKey('x', 'name')))).toEqual( + createInlineValue('Alice') + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/JoinReducer.test.js` around lines 595 - 617, Replace the fragile internal-access assertion that reads cloned.prop.get(encodePropKey('x','name'))?.value with the public contract helper lwwValue: call lwwValue(cloned.prop.get(encodePropKey('x','name'))) and assert that it equals createInlineValue('Alice'); i.e., use lwwValue(...) in place of accessing .value so the test depends on the LWW API (refer to functions lwwValue, encodePropKey, createInlineValue and the cloned.prop map).test/unit/domain/services/strand/ConflictAnalyzerService.test.js (2)
1509-1531: Shared state mutation pattern for testing legacy behavior.Directly mutating
JoinReducer.OP_STRATEGIESto test receipt name variations is acceptable here since cleanup is properly handled in the finally block. Consider documenting this pattern with a brief comment to clarify intent for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/strand/ConflictAnalyzerService.test.js` around lines 1509 - 1531, Add a brief comment explaining the intentional temporary mutation of shared state for testing legacy behavior: annotate the section where JoinReducer.OP_STRATEGIES is accessed and nodeAddStrategy.receiptName is set to 'UnsupportedEffect' (and restored in the finally block) to clarify this pattern to future maintainers; mention that reduceV5 is being spied on (reduceSpy) and that cleanup is handled in the finally block to justify the direct mutation.
2028-2028: Consider movingVALID_KINDS_SETto the top of the file.The constant is defined at the end but used in line 1829. While this works due to how JavaScript modules are parsed before test execution, placing it near the imports or at the top of the test suite improves readability and follows common conventions.
📝 Suggested placement
Move this line near the top of the file, after the imports:
import StrandService from '../../../../../src/domain/services/strand/StrandService.js'; +const VALID_KINDS_SET = new Set(['supersession', 'eventual_override', 'redundancy']); + // ── Deterministic helpers ─────────────────────────────────────────────────────And remove it from line 2028.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/strand/ConflictAnalyzerService.test.js` at line 2028, Move the VALID_KINDS_SET constant definition up to the top of the test file (immediately after the import block) so it’s defined before its uses in the test suite; update any references to use this single definition and remove the duplicate definition currently located at the bottom of the file (symbol: VALID_KINDS_SET).test/unit/domain/WarpApp.delegation.test.js (1)
52-65: Consider adding explicit type annotations for mock variables.The static analysis flags implicit
anytypes formockRuntimeandmockCore. While this is common in test files, adding JSDoc type annotations would improve type safety and IDE support.The private property access (
_runtime) and constructor argument type mismatch are intentional for testing delegation—this is an acceptable testing pattern.🔧 Optional: Add type annotations
describe('WarpApp delegation', () => { /** `@type` {WarpApp} */ let app; + /** `@type` {ReturnType<typeof createMockRuntime>} */ let mockRuntime; + /** `@type` {ReturnType<typeof createMockCore>} */ let mockCore; beforeEach(() => { mockRuntime = createMockRuntime(); mockCore = createMockCore(); // Construct WarpApp with a mock core that also acts as runtime - app = new WarpApp(mockCore); + app = new WarpApp(/** `@type` {any} */ (mockCore)); // Override _runtime() to return our mock runtime (which has all the methods) - app._runtime = () => mockRuntime; + /** `@type` {any} */ (app)._runtime = () => mockRuntime; // Override core() to return our mock core - app.core = () => mockCore; + /** `@type` {any} */ (app).core = () => mockCore; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/WarpApp.delegation.test.js` around lines 52 - 65, Add JSDoc type annotations for the test mocks to avoid implicit any: annotate mockRuntime and mockCore with their expected interfaces returned by createMockRuntime() and createMockCore() (e.g., /** `@type` {Runtime} */ or the appropriate mock interfaces) above their declarations and, if helpful, add /** `@type` {WarpApp} */ for app; ensure references to createMockRuntime, createMockCore, WarpApp, app._runtime, and app.core remain unchanged—only add JSDoc comments so the test file gains proper types for IDEs and static analysis.test/unit/domain/services/GraphTraversal.test.js (1)
287-299: Consider: AbortSignal test may not exercise the 1000-node check.The test creates 999 edges from
root, totaling 1000 nodes. However, withac.abort()called before execution starts, the signal is already aborted when the method begins. If the implementation checkssignal.abortedat the start of execution (before the 1000-node interval check), this test verifies initial abort detection rather than the periodic check.This is still valid coverage, but the test name "checks AbortSignal every thousand visited nodes" may be misleading. Consider renaming to clarify it tests abort detection at start, or increase the graph size and use a delayed abort if the periodic check is the intended target.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/GraphTraversal.test.js` around lines 287 - 299, The test name is misleading because ac.abort() is called before traversal starts so it only verifies initial abort detection; either rename the test to indicate it checks abort-at-start for GraphTraversal.shortestPath, or modify it to actually exercise the periodic "every 1000 visited nodes" check by creating >1000 nodes (e.g., 1001+) and aborting the controller after traversal begins (use a small setTimeout or abort on first few node visits) so the implementation's periodic signal check is triggered; update references in the spec accordingly to mention GraphTraversal.shortestPath and the AbortSignal periodic check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/coverage-ratchet.js`:
- Around line 17-18: The TS4111 error arises from using dot notation on the
index-signature typed env (noPropertyAccessFromIndexSignature); update the
function shouldAutoUpdateCoverageRatchet to read the variable via bracket
notation (env['GIT_WARP_UPDATE_COVERAGE_RATCHET']) instead of
env.GIT_WARP_UPDATE_COVERAGE_RATCHET while keeping the default parameter env =
process.env so the value check still returns whether it equals '1'.
In `@test/unit/domain/services/WormholeService.test.js`:
- Around line 759-767: The test in the "throws when fromSha is not a string"
case expects an incorrect validation message missing the leading quote; update
the expected string in the expect(...).toThrow assertion for deserializeWormhole
to "'fromSha' must be a string" (i.e., include the leading single quote before
fromSha) so the asserted error message matches the actual validator output.
---
Nitpick comments:
In `@test/unit/domain/services/AuditReceiptService.test.js`:
- Around line 269-273: The test uses Number.MAX_SAFE_INTEGER + 1 which can be
ambiguous due to floating-point precision; update the failing timestamp in the
test (set on f.timestamp in AuditReceiptService.test.js created via
validFields()) to a clearly unsafe value such as 2 ** 53 + 1 or
Number.MAX_SAFE_INTEGER + 100 so buildReceiptRecord(f) deterministically rejects
it; adjust the expect call to remain the same but assign f.timestamp = 2 ** 53 +
1 (or +100) before calling buildReceiptRecord.
In `@test/unit/domain/services/GraphTraversal.test.js`:
- Around line 287-299: The test name is misleading because ac.abort() is called
before traversal starts so it only verifies initial abort detection; either
rename the test to indicate it checks abort-at-start for
GraphTraversal.shortestPath, or modify it to actually exercise the periodic
"every 1000 visited nodes" check by creating >1000 nodes (e.g., 1001+) and
aborting the controller after traversal begins (use a small setTimeout or abort
on first few node visits) so the implementation's periodic signal check is
triggered; update references in the spec accordingly to mention
GraphTraversal.shortestPath and the AbortSignal periodic check.
In `@test/unit/domain/services/IncrementalIndexUpdater.test.js`:
- Around line 732-882: The tests repeat creating state/tree/updater and various
caches; extract a small test harness helper (e.g., buildGuardHarness) that
constructs and returns the common fixtures (state, tree, updater, metaCache,
fwdCache, revCache, out, cache) and use it in tests that exercise
_handleNodeRemove, _purgeNodeEdges, _handleEdgeAdd, _handleEdgeRemove,
_flushEdgeShards, _loadLabels, _getOrBuildAliveEdgeAdjacency and
_removeEdgeKeyFromAdjacency to eliminate duplication and make each test body
only set up the minimal per-test inputs and assertions.
In `@test/unit/domain/services/JoinReducer.test.js`:
- Around line 595-617: Replace the fragile internal-access assertion that reads
cloned.prop.get(encodePropKey('x','name'))?.value with the public contract
helper lwwValue: call lwwValue(cloned.prop.get(encodePropKey('x','name'))) and
assert that it equals createInlineValue('Alice'); i.e., use lwwValue(...) in
place of accessing .value so the test depends on the LWW API (refer to functions
lwwValue, encodePropKey, createInlineValue and the cloned.prop map).
In `@test/unit/domain/services/Observer.test.js`:
- Line 640: Test currently asserts exact order from view.getNodes() which is not
guaranteed; change the assertion in the Observer test to be order-insensitive by
verifying the returned array contains the expected items regardless of order
(for example use an unordered matcher like expect.arrayContaining([...])
combined with length check, or sort both actual and expected before comparing)
and update the assertion referencing view.getNodes() and the expected
['user:bob','user:alice'] accordingly.
In `@test/unit/domain/services/strand/ConflictAnalyzerService.test.js`:
- Around line 1509-1531: Add a brief comment explaining the intentional
temporary mutation of shared state for testing legacy behavior: annotate the
section where JoinReducer.OP_STRATEGIES is accessed and
nodeAddStrategy.receiptName is set to 'UnsupportedEffect' (and restored in the
finally block) to clarify this pattern to future maintainers; mention that
reduceV5 is being spied on (reduceSpy) and that cleanup is handled in the
finally block to justify the direct mutation.
- Line 2028: Move the VALID_KINDS_SET constant definition up to the top of the
test file (immediately after the import block) so it’s defined before its uses
in the test suite; update any references to use this single definition and
remove the duplicate definition currently located at the bottom of the file
(symbol: VALID_KINDS_SET).
In `@test/unit/domain/services/VisibleStateScopeV1.test.js`:
- Around line 115-124: The test title is misleading — update the 'it'
description in the test containing normalizeVisibleStateScopeV1 and
nodeIdInVisibleStateScope to reflect that it verifies node-level
inclusion/exclusion (not edge filtering); change the string from something like
"matches include-empty rules and filters edges by endpoint visibility" to a
clearer title such as "matches include-empty rules and excludes node IDs by
prefix" so failures correctly indicate the asserted behavior.
- Around line 149-155: The test assumes a stable ordering from
createStateReaderV5 by asserting reader.getNodes() equals an exact array, which
can be flaky; change the assertion for getNodes() in VisibleStateScopeV1.test.js
to be order-insensitive (e.g., compare sorted arrays or use a
set/array-contains-same-members style assertion) while keeping the existing
getEdges() check if edge order is contractual; update the test line that calls
reader.getNodes() to sort both actual and expected node lists or use
expect.arrayContaining/containSameMembers equivalent so tests no longer fail due
to non-deterministic node ordering.
In `@test/unit/domain/services/VisibleStateTransferPlannerV5.test.js`:
- Around line 196-213: The test currently uses the same reader instance for both
source and target which can hide shared-reference bugs; create two separate but
equivalent readers (e.g. sourceReader and targetReader) by calling createReader
twice with the same fixture data and use those when calling
planVisibleStateTransferV5 (keep the loadNodeContent and loadEdgeContent mocks
unchanged) so the test asserts idempotency using distinct instances.
In `@test/unit/domain/WarpApp.delegation.test.js`:
- Around line 52-65: Add JSDoc type annotations for the test mocks to avoid
implicit any: annotate mockRuntime and mockCore with their expected interfaces
returned by createMockRuntime() and createMockCore() (e.g., /** `@type` {Runtime}
*/ or the appropriate mock interfaces) above their declarations and, if helpful,
add /** `@type` {WarpApp} */ for app; ensure references to createMockRuntime,
createMockCore, WarpApp, app._runtime, and app.core remain unchanged—only add
JSDoc comments so the test file gains proper types for IDEs and static analysis.
🪄 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: b9186765-3dbf-4a66-ab3e-2c46f34527fd
📒 Files selected for processing (78)
AGENTS.mddocs/design/0010-100-percent-coverage/100-percent-coverage.mddocs/method/backlog/asap/PROTO_conflict-analyzer-pipeline-decomposition.mddocs/method/backlog/asap/PROTO_dag-pathfinding-algorithm-split.mddocs/method/backlog/asap/PROTO_strand-service-boundary-split.mddocs/method/backlog/asap/PROTO_warpruntime-open-options-class.mddocs/method/backlog/asap/VIZ_cut-git-warp-visualization-surface-in-favor-of-warp-ttd.mddocs/method/backlog/bad-code/PROTO_bitmap-neighbor-provider-dead-false-branch.mddocs/method/backlog/bad-code/PROTO_conflict-analyzer-dead-branches.mddocs/method/backlog/bad-code/PROTO_dag-pathfinding-raw-error.mddocs/method/backlog/bad-code/PROTO_incremental-index-updater-null-proto-rewrap-dead-branch.mddocs/method/backlog/bad-code/PROTO_inmemory-graph-adapter-default-hash-unavailable-branch.mddocs/method/backlog/bad-code/PROTO_join-reducer-import-time-strategy-validation-residue.mddocs/method/backlog/bad-code/PROTO_materialize-controller-seek-cache-error-opacity.mddocs/method/backlog/bad-code/PROTO_roaring-loader-fallback-opacity.mddocs/method/backlog/bad-code/PROTO_state-diff-private-helper-residue.mddocs/method/backlog/bad-code/PROTO_strand-service-dead-branches.mddocs/method/backlog/bad-code/PROTO_streaming-bitmap-index-builder-serialization-tail.mddocs/method/backlog/bad-code/PROTO_trust-record-service-unreachable-exhausted-tails.mddocs/method/backlog/bad-code/PROTO_wormhole-service-defensive-tail-branches.mddocs/method/retro/0010-100-percent-coverage/100-percent-coverage.mdpackage.jsonscripts/coverage-ratchet.jstest/unit/domain/WarpApp.delegation.test.jstest/unit/domain/WarpCore.content.test.jstest/unit/domain/WarpGraph.coverageGaps.test.jstest/unit/domain/WarpGraph.queryBuilder.test.jstest/unit/domain/WarpGraph.test.jstest/unit/domain/WarpGraph.traverse.test.jstest/unit/domain/errors/index.test.jstest/unit/domain/services/AuditReceiptService.test.jstest/unit/domain/services/AuditVerifierService.test.jstest/unit/domain/services/BitmapIndexReader.test.jstest/unit/domain/services/BitmapNeighborProvider.test.jstest/unit/domain/services/CheckpointService.edgeCases.test.jstest/unit/domain/services/CheckpointService.test.jstest/unit/domain/services/DagPathFinding.test.jstest/unit/domain/services/GitGraphAdapter.test.jstest/unit/domain/services/GraphTraversal.test.jstest/unit/domain/services/GraphTraversal.transitiveClosure.test.jstest/unit/domain/services/GraphTraversal.transitiveReduction.test.jstest/unit/domain/services/IncrementalIndexUpdater.test.jstest/unit/domain/services/JoinReducer.test.jstest/unit/domain/services/JoinReducer.trackDiff.test.jstest/unit/domain/services/Observer.test.jstest/unit/domain/services/StateDiff.test.jstest/unit/domain/services/StreamingBitmapIndexBuilder.test.jstest/unit/domain/services/VisibleStateScopeV1.test.jstest/unit/domain/services/VisibleStateTransferPlannerV5.test.jstest/unit/domain/services/WormholeService.test.jstest/unit/domain/services/controllers/MaterializeController.test.jstest/unit/domain/services/state/StateHashService.test.jstest/unit/domain/services/strand/ConflictAnalyzerService.test.jstest/unit/domain/services/strand/StrandService.test.jstest/unit/domain/stream/WarpStream.test.jstest/unit/domain/trust/TrustRecordService.test.jstest/unit/domain/trust/verdict.test.jstest/unit/domain/types/WarpErrors.test.jstest/unit/domain/types/WarpOptions.test.jstest/unit/domain/types/WarpPersistence.test.jstest/unit/domain/utils/callInternalRuntimeMethod.test.jstest/unit/domain/utils/defaultCodec.test.jstest/unit/domain/utils/defaultCrypto.unavailable.test.jstest/unit/domain/utils/defaultTrustCrypto.test.jstest/unit/domain/utils/defaultTrustCrypto.unavailable.test.jstest/unit/domain/utils/matchGlob.test.jstest/unit/domain/utils/roaring.test.jstest/unit/domain/utils/streamUtils.test.jstest/unit/domain/warp/PatchSession.test.jstest/unit/infrastructure/adapters/CborCheckpointStoreAdapter.test.jstest/unit/infrastructure/adapters/GitGraphAdapter.coverage.test.jstest/unit/infrastructure/adapters/InMemoryBlobStorageAdapter.test.jstest/unit/infrastructure/adapters/InMemoryGraphAdapter.test.jstest/unit/ports/IndexStorePort.test.jstest/unit/ports/NeighborProviderPort.test.jstest/unit/ports/PatchJournalPort.test.jstest/unit/scripts/coverage-ratchet.test.jsvitest.config.js
✅ Files skipped from review due to trivial changes (20)
- docs/method/backlog/bad-code/PROTO_wormhole-service-defensive-tail-branches.md
- docs/method/backlog/bad-code/PROTO_join-reducer-import-time-strategy-validation-residue.md
- docs/method/backlog/bad-code/PROTO_trust-record-service-unreachable-exhausted-tails.md
- test/unit/domain/services/JoinReducer.trackDiff.test.js
- docs/method/backlog/bad-code/PROTO_dag-pathfinding-raw-error.md
- docs/method/backlog/asap/PROTO_warpruntime-open-options-class.md
- test/unit/domain/services/GraphTraversal.transitiveReduction.test.js
- docs/method/backlog/bad-code/PROTO_streaming-bitmap-index-builder-serialization-tail.md
- docs/method/backlog/bad-code/PROTO_incremental-index-updater-null-proto-rewrap-dead-branch.md
- docs/method/backlog/bad-code/PROTO_bitmap-neighbor-provider-dead-false-branch.md
- docs/method/backlog/bad-code/PROTO_strand-service-dead-branches.md
- docs/method/backlog/bad-code/PROTO_inmemory-graph-adapter-default-hash-unavailable-branch.md
- docs/method/backlog/asap/PROTO_strand-service-boundary-split.md
- test/unit/domain/services/BitmapNeighborProvider.test.js
- docs/method/backlog/asap/PROTO_dag-pathfinding-algorithm-split.md
- docs/method/backlog/asap/VIZ_cut-git-warp-visualization-surface-in-favor-of-warp-ttd.md
- docs/method/backlog/bad-code/PROTO_conflict-analyzer-dead-branches.md
- docs/method/retro/0010-100-percent-coverage/100-percent-coverage.md
- test/unit/domain/services/StateDiff.test.js
- test/unit/domain/services/controllers/MaterializeController.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Comment on lines
+17
to
+18
| export function shouldAutoUpdateCoverageRatchet(env = process.env) { | ||
| return env.GIT_WARP_UPDATE_COVERAGE_RATCHET === '1'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify usages of the ratchet env key and confirm bracket-notation fix was applied.
rg -n -C2 "GIT_WARP_UPDATE_COVERAGE_RATCHET" scripts/coverage-ratchet.js vitest.config.js package.jsonRepository: git-stunts/git-warp
Length of output: 849
🏁 Script executed:
fd -name "tsconfig*.json" -o -name "jsconfig*.json"Repository: git-stunts/git-warp
Length of output: 293
🏁 Script executed:
cat tsconfig.json 2>/dev/null | head -50Repository: git-stunts/git-warp
Length of output: 313
🏁 Script executed:
rg -n "noPropertyAccessFromIndexSignature" tsconfig.jsonRepository: git-stunts/git-warp
Length of output: 45
🏁 Script executed:
cat tsconfig.base.jsonRepository: git-stunts/git-warp
Length of output: 969
🏁 Script executed:
rg "GIT_WARP_UPDATE_COVERAGE_RATCHET" --type js -A2 -B2 scripts/coverage-ratchet.jsRepository: git-stunts/git-warp
Length of output: 193
Fix TS4111 blocker by using bracket notation for env access.
Line 18 fails typecheck (tsc --noEmit) under noPropertyAccessFromIndexSignature rules enabled in tsconfig. The process.env parameter has an index signature, and dot notation access violates this strict setting.
Proposed fix
export function shouldAutoUpdateCoverageRatchet(env = process.env) {
- return env.GIT_WARP_UPDATE_COVERAGE_RATCHET === '1';
+ return env['GIT_WARP_UPDATE_COVERAGE_RATCHET'] === '1';
}📝 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
| export function shouldAutoUpdateCoverageRatchet(env = process.env) { | |
| return env.GIT_WARP_UPDATE_COVERAGE_RATCHET === '1'; | |
| export function shouldAutoUpdateCoverageRatchet(env = process.env) { | |
| return env['GIT_WARP_UPDATE_COVERAGE_RATCHET'] === '1'; | |
| } |
🧰 Tools
🪛 GitHub Actions: Release Preflight (PR)
[error] 18-18: TypeScript typecheck failed (tsc --noEmit). TS4111: Property 'GIT_WARP_UPDATE_COVERAGE_RATCHET' comes from an index signature, so it must be accessed with ['GIT_WARP_UPDATE_COVERAGE_RATCHET'].
🪛 GitHub Check: preflight
[failure] 18-18:
Property 'GIT_WARP_UPDATE_COVERAGE_RATCHET' comes from an index signature, so it must be accessed with ['GIT_WARP_UPDATE_COVERAGE_RATCHET'].
🪛 GitHub Check: type-firewall
[failure] 18-18:
Property 'GIT_WARP_UPDATE_COVERAGE_RATCHET' comes from an index signature, so it must be accessed with ['GIT_WARP_UPDATE_COVERAGE_RATCHET'].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/coverage-ratchet.js` around lines 17 - 18, The TS4111 error arises
from using dot notation on the index-signature typed env
(noPropertyAccessFromIndexSignature); update the function
shouldAutoUpdateCoverageRatchet to read the variable via bracket notation
(env['GIT_WARP_UPDATE_COVERAGE_RATCHET']) instead of
env.GIT_WARP_UPDATE_COVERAGE_RATCHET while keeping the default parameter env =
process.env so the value check still returns whether it equals '1'.
Comment on lines
+759
to
+767
| it('throws when fromSha is not a string', () => { | ||
| expect(() => deserializeWormhole({ | ||
| fromSha: 123, | ||
| toSha: 'def456', | ||
| writerId: 'alice', | ||
| patchCount: 1, | ||
| payload: { version: 1, patches: [] }, | ||
| })).toThrow("fromSha' must be a string"); | ||
| }); |
There was a problem hiding this comment.
Fix typo in the expected fromSha validation message.
At Line 766, the expected message is missing the leading quote before fromSha, which makes the contract less clear.
Proposed fix
- })).toThrow("fromSha' must be a string");
+ })).toThrow("'fromSha' must be a string");📝 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('throws when fromSha is not a string', () => { | |
| expect(() => deserializeWormhole({ | |
| fromSha: 123, | |
| toSha: 'def456', | |
| writerId: 'alice', | |
| patchCount: 1, | |
| payload: { version: 1, patches: [] }, | |
| })).toThrow("fromSha' must be a string"); | |
| }); | |
| it('throws when fromSha is not a string', () => { | |
| expect(() => deserializeWormhole({ | |
| fromSha: 123, | |
| toSha: 'def456', | |
| writerId: 'alice', | |
| patchCount: 1, | |
| payload: { version: 1, patches: [] }, | |
| })).toThrow("'fromSha' must be a string"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/services/WormholeService.test.js` around lines 759 - 767,
The test in the "throws when fromSha is not a string" case expects an incorrect
validation message missing the leading quote; update the expected string in the
expect(...).toThrow assertion for deserializeWormhole to "'fromSha' must be a
string" (i.e., include the leading single quote before fromSha) so the asserted
error message matches the actual validator output.
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
npm run test:coverageupdates the checked-in thresholdWarpApp,WarpCore,WarpRuntime, and a broad sweep of remaining high-yield services, adapters, and utilities/* v8 ignore */warp-ttdWhy
This cycle was the precondition for the next refactor wave.
The repo had large amounts of untested, high-risk code concentrated in controllers, strand services, and orchestration surfaces. The goal was to make the coverage number honest, retire the biggest blind spots, and pin current behavior before starting the "break up the gods" work.
Result
The cycle did not reach literal 100% line coverage. It reached an honest natural break instead:
Validation
npm run test:coverage/* v8 ignore */suppressions added for the residual missesdocs/design/0010-100-percent-coverage/100-percent-coverage.md,docs/method/retro/0010-100-percent-coverage/100-percent-coverage.md)Summary by CodeRabbit
Documentation
Tests
Chores