v3.0.0 #5
Open
v3.0.0 #5
Conversation
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
- GitGraphAdapter now retries transient Git failures using @git-stunts/alfred with exponential backoff and decorrelated jitter - Extract CachedValue utility from HealthCheckService for reusable TTL caching - BitmapIndexReader logs warning when ID-to-SHA cache exceeds 1M entries - Refactor TraversalService path reconstruction into unified helpers - Add Memory Considerations section to README - Update Docker build context to include alfred sibling package
BREAKING CHANGE: This release introduces managed mode as the recommended way to use EmptyGraph. Nodes created without managed mode or explicit ref management may be garbage collected by Git. ## Durability (Core Fix) - Add EmptyGraph.open() static factory with mode='managed' - Add GraphRefManager service for ref/anchor management - Add automatic anchor commits for disconnected subgraphs - Add fast-forward optimization (skip anchors for linear history) - Add isAncestor() to GitGraphAdapter for ancestry checking ## Batching API - Add beginBatch() for efficient bulk writes - Add GraphBatch with octopus anchoring on commit() - Add compactAnchors() utility for cleanup ## Validation - Add EmptyMessageError for empty message rejection - Reject empty messages at write time (fixes "ghost nodes") ## Index Improvements - Add canonical JSON checksums (SHARD_VERSION=2) - Add backward compatibility for v1 shards - Add SUPPORTED_SHARD_VERSIONS array ## Cancellation - Add AbortSignal to all TraversalService methods - Add AbortSignal to StreamingBitmapIndexBuilder finalization - Fix signal propagation in IndexRebuildService ## Documentation - Add SEMANTICS.md (durability contract) - Add docs/ANCHORING.md (anchor mechanics) - Add docs/ARCHITECTURE.md (hexagonal design) - Add docs/WARP-ROADMAP.md (v4/v5 evolution path) - Update README with durability guide - Update CHANGELOG for v3.0.0 ## Tests - Add 26 managed mode durability tests - Add 12 manual mode tests - Add 6 empty message validation tests - Add 9 shard versioning tests
📝 WalkthroughWalkthroughThis PR introduces managed mode durability for EmptyGraph with automatic ref synchronization and anchor commits. Key additions include GraphRefManager for ref management, GraphBatch for bulk operations, EmptyMessageError validation, SHARD_VERSION bump to 2 with backward compatibility, and AbortSignal-based cancellation throughout traversal APIs. Extensive documentation details durability contracts, architecture, and anchoring strategies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EmptyGraph
participant GraphBatch
participant GraphRefManager
participant GitAdapter
participant Git
Client->>EmptyGraph: beginBatch()
EmptyGraph->>GraphBatch: new GraphBatch(...)
Client->>GraphBatch: createNode(msg, parents) x N
GraphBatch->>GraphBatch: accumulate SHAs
Client->>GraphBatch: commit()
GraphBatch->>GitAdapter: commitNode(octopus anchor payload)
GitAdapter->>Git: git commit (anchor with N parents)
Git-->>GitAdapter: anchorSha
GraphBatch->>GraphRefManager: syncHead(ref, anchorSha)
GraphRefManager->>GitAdapter: updateRef(ref, anchorSha)
GitAdapter->>Git: git update-ref
Git-->>GitAdapter: success
GraphRefManager-->>GraphBatch: {updated: true, anchor: true}
GraphBatch-->>Client: commit result
sequenceDiagram
participant Client
participant EmptyGraph
participant GraphRefManager
participant GitAdapter
participant Git
Client->>EmptyGraph: sync(newSha)
EmptyGraph->>GraphRefManager: syncHead(ref, newSha)
GraphRefManager->>GitAdapter: readRef(ref)
GitAdapter->>Git: git rev-parse
Git-->>GitAdapter: currentSha (or null)
alt Ref missing
GraphRefManager->>GitAdapter: updateRef(ref, newSha)
GitAdapter->>Git: git update-ref
Git-->>GitAdapter: success
GraphRefManager-->>Client: {updated: true, anchor: false}
else Current is ancestor of new (fast-forward)
GraphRefManager->>GitAdapter: updateRef(ref, newSha)
GitAdapter->>Git: git update-ref
Git-->>GitAdapter: success
GraphRefManager-->>Client: {updated: true, anchor: false}
else Divergent history
GraphRefManager->>GitAdapter: commitNode({_type: 'anchor'}, [currentSha, newSha])
GitAdapter->>Git: git commit (octopus)
Git-->>GitAdapter: anchorSha
GraphRefManager->>GitAdapter: updateRef(ref, anchorSha)
GitAdapter->>Git: git update-ref
Git-->>GitAdapter: success
GraphRefManager-->>Client: {updated: true, anchor: true}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
3-3:⚠️ Potential issue | 🟡 MinorVersion mismatch with PR title.
The PR is titled "v3.0.0" but
package.jsonstill has"version": "2.5.0". If this is a major version release, the version should be updated to3.0.0.src/infrastructure/adapters/GitGraphAdapter.js (1)
67-76:⚠️ Potential issue | 🔴 CriticalRemove retry logic from
commitNodeto prevent duplicate commits.
commit-treegenerates unique SHAs for each execution due to embedded timestamps. Retrying on transient errors (e.g.,'cannot lock ref') creates duplicate commits with different SHAs, corrupting graph state and breaking anchor commit logic.Replace with direct execution (no retry)
- const oid = await this._executeWithRetry({ args }); + const oid = await this.plumbing.execute({ args });
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 10-11: Update the release date in the changelog header "## [3.0.0]
- 2025-01-30" so it is chronologically consistent with the 2026 entries below
(for example change to "2026-01-30" or the correct 2026 date you intend); edit
the version header line only to reflect the correct year while preserving the
"## [3.0.0]" text.
In `@Dockerfile`:
- Around line 9-14: The Dockerfile's COPY alfred ../alfred step fails in CI
because the alfred directory isn't in the build context; update the build to
remove the local sibling dependency or ensure CI provides it: either make CI
check out the monorepo before docker build (so the Dockerfile's COPY alfred
../alfred succeeds), or stop copying the local package and depend on the
published package (publish `@git-stunts/alfred` to npm and update package.json in
empty-graph), or add a CI step that clones/copies the alfred repo into the build
context prior to running the Dockerfile; locate the COPY alfred ../alfred and
related COPY empty-graph / RUN npm install lines in the Dockerfile and apply one
of these fixes accordingly.
In `@docs/ARCHITECTURE.md`:
- Around line 409-426: The markdown tables under the "Performance
Characteristics" section (headers and "Memory characteristics" table) use
compact pipe formatting that triggers MD060; update each table row and header so
there is a single space on both sides of every pipe (e.g., change "|Write
(createNode)|O(1)|Append-only commit|" to "| Write (createNode) | O(1) |
Append-only commit |") and ensure the separator row uses spaced pipes too (e.g.,
"| --- | --- | --- |"); apply this change to both tables in the same section so
markdownlint no longer flags MD060.
- Around line 52-90: The fenced diagram block (showing EmptyGraph, Domain
Services like GraphService/IndexRebuild/TraversalService, Ports like
GraphPersistence/IndexStoragePort, and Adapters like
GitGraphAdapter/ConsoleLogger) lacks a language identifier; update the opening
triple-backtick for this and all other fenced blocks in ARCHITECTURE.md to
include an appropriate language tag (e.g., text, mermaid, json, or javascript)
so markdownlint MD040 is satisfied—locate the block containing "EmptyGraph
(index.js)" and similar blocks and change ``` to ```text (or a more specific
language where appropriate) for each fence.
In `@index.js`:
- Around line 238-249: In the static open method of EmptyGraph (static async
open), add a validation that when mode === 'managed' the ref parameter must be
provided (not undefined/null/empty); if it's missing, throw a clear error (e.g.,
"ref is required for managed mode") before constructing GraphRefManager or
assigning graph._ref, so callers fail fast instead of later in persistence;
update the guard around creating GraphRefManager (and any assignments that
assume ref) to rely on this validation.
- Around line 359-373: The docs claim sync(sha) accepts an optional sha but the
implementation throws when sha is missing; either update the docs to state sha
is required or implement the fallback: in async sync(sha) (method name: sync)
remove the unconditional throw for missing sha and, when sha is falsy, obtain
the “last created node” SHA from the ref manager (use/add a method on
this._refManager such as getLastCreatedSha/getLatestNodeSha or similar) and
assign it to sha, then if sha is still falsy throw a clear error; finally call
and return this._refManager.syncHead(this._ref, sha). Ensure you reference
this._refManager, this._ref and syncHead in the change.
- Around line 346-354: The current managed autoSync behavior only calls
this._refManager.syncHead with the last SHA returned from
this.service.createNodes, which can leave earlier, potentially disconnected SHAs
unreachable and subject to GC; modify the logic so when this._mode === 'managed'
&& this._autoSync === 'onWrite' && this._refManager and shas.length > 0, you
iterate over all SHAs returned by createNodes (the shas array) and call
this._refManager.syncHead(this._ref, sha) for each sha (or batch them if
supported) to ensure every created node is anchored and reachable.
- Around line 740-796: compactAnchors() is over the max-lines limit; refactor by
extracting node collection and tip discovery into small helper methods. Move the
loop that iterates this.iterateNodes(...) and builds anchors and realNodes into
a helper like collectAnchorsAndRealNodes(ref) that returns { anchors, realNodes
}, and move the child-detection/tips logic into another helper like
findTipsFromRealNodes(realNodes) that returns tips. Update compactAnchors to
call collectAnchorsAndRealNodes(targetRef) and findTipsFromRealNodes(realNodes),
then keep only the commitNode/updateRef and return logic in compactAnchors;
ensure you reuse _persistence.commitNode and _persistence.updateRef as before
and preserve returned shape.
- Around line 63-84: The nested loops in _findDisconnectedTips cause max-depth
lint errors; extract the inner ancestor-check into a small async helper (e.g.,
_isAncestorOfAnother(candidate)) that iterates this._createdShas and returns
true as soon as this._graph._persistence.isAncestor(candidate, other) is true,
then in _findDisconnectedTips replace the inner loop with: if (!(await
this._isAncestorOfAnother(candidate))) tips.push(candidate); this flattens the
control flow while keeping references to this._createdShas and
this._graph._persistence.isAncestor.
In `@README.md`:
- Around line 245-249: Normalize spacing around pipe characters in the
Performance Considerations table so the Markdown linter (MD060) is satisfied:
ensure the header row ("Pattern", "Anchor Overhead", "Notes") and the separator
row ("---------", "-----------------", "-------") and each data row have a
single space on both sides of every pipe, and make the columns consistently
aligned (e.g., change rows like "|Pattern|Anchor Overhead|Notes|" or
inconsistent spacing to "| Pattern | Anchor Overhead | Notes |" and likewise for
the separator and data rows).
- Around line 68-78: The Markdown table in README.md violates markdownlint MD060
due to inconsistent pipe spacing; fix by ensuring every row's pipe separators
align with the header separator row (the "| ------------------------ |
---------------------------------------------------- |
--------------------------------------- |
---------------------------------------------------------------- |" line) —
adjust each data row so column pipes match that header style and spacing
consistently for all rows (Network connectivity, Replication model, Write
patterns, Existing ecosystem, Audit requirements, Graph size, Query complexity,
Data deletion, Philosophy) so the table is properly aligned and markdownlint
passes.
In `@src/domain/services/BitmapIndexBuilder.js`:
- Around line 21-29: Lint flags use of string concatenation in
canonicalStringify; replace concatenations with template literals. Update the
array branch to return a template literal like `[\${...}]` using
obj.map(canonicalStringify).join(',') and update the object branch to build the
string with template literals when composing keys and values (use
`\${JSON.stringify(k)}:\${canonicalStringify(obj[k])}` inside the keys.map) and
wrap the joined result in `{...}`; keep the existing sorting of keys and commas
from join to preserve output.
- Line 11: Bitmap shard version is inconsistent: BitmapIndexBuilder exports
SHARD_VERSION = 2 while StreamingBitmapIndexBuilder uses/exported version 1,
causing StreamingBitmapIndexBuilder's version validation to reject shards
created by BitmapIndexBuilder; locate the SHARD_VERSION export in
StreamingBitmapIndexBuilder and change it to 2 (or otherwise update the version
constant used in its validation logic) so both builders share the same
SHARD_VERSION value, ensuring compatibility between BitmapIndexBuilder and
StreamingBitmapIndexBuilder.
In `@src/domain/services/BitmapIndexReader.js`:
- Around line 31-39: The canonicalStringify function currently builds strings
via '+' concatenation (e.g., '[' + obj.map(...).join(',') + ']', '{' +
keys.map(...).join(',') + '}', and JSON.stringify(k) + ':' +
canonicalStringify(obj[k])); replace those concatenations with non-concatenation
constructions (for example use template literals or build an array and join)
while preserving exact output formatting and ordering; update the
implementations inside canonicalStringify (the Array.isArray branch and the
object branch where keys.map is used and the key:value assembly) to remove '+'
concatenation but keep the same separators and quotes.
In `@src/domain/services/GraphRefManager.js`:
- Around line 102-152: The syncHead function is too long—extract the three main
branches into private helpers: one to handle the "no current tip / create ref"
branch (call it e.g. _handleCreateRef), one for the "fast-forward" branch
(_handleFastForward) and one for the "divergent history / anchor" branch
(_handleCreateAnchor); each helper should accept the inputs it needs (ref,
newTipSha, currentTip, startTime or compute duration inside) and perform the
calls to this.persistence.updateRef, this.logger.debug and return the same shape
({updated, anchor, sha}) so syncHead becomes a thin coordinator that calls
readHead, then delegates to _handleCreateRef, _handleFastForward (after calling
this.persistence.isAncestor) or _handleCreateAnchor (which should call
this.createAnchor and updateRef). Ensure helpers preserve existing log keys
(operation:'syncHead', ref, sha, parents/previousTip, anchor, durationMs) and
reuse methods readHead, createAnchor, persistence.isAncestor,
persistence.updateRef, and logger.debug.
- Around line 35-41: In the GraphRefManager constructor validate that the
provided persistence adapter implements the required API (at minimum isAncestor)
before assigning it; add a short validation block in constructor({ persistence,
logger = new NoOpLogger() }) that defines an array of required method names
(including "isAncestor") and checks each exists and is a function on
persistence, and if any are missing throw a clear Error listing the missing
method(s) so incomplete adapters fail fast.
In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 383-401: The isAncestor method currently catches all errors and
returns false; change it to only treat a non-ancestor git exit (exit code 1) as
false and rethrow any other errors so real failures aren't swallowed. Inside
isAncestor (referencing isAncestor, _executeWithRetry and _validateOid), catch
the error object, inspect its fields (e.g., err.code or err.exitCode and
fallback to message parsing as done in readRef elsewhere in this file) and if
the code equals 1 return false, otherwise throw the error so callers can handle
plumbing/repo failures.
In `@test/unit/domain/services/BitmapIndexReader.test.js`:
- Line 4: Remove the unused named import SHARD_VERSION from the import statement
that brings in BitmapIndexBuilder (currently: import BitmapIndexBuilder, {
SHARD_VERSION } from ...). Update the import to only import the default
BitmapIndexBuilder (e.g., import BitmapIndexBuilder from ...), ensuring no other
references to SHARD_VERSION exist in BitmapIndexReader.test.js; run linter/tests
to confirm the unused-import warning is resolved.
In `@test/unit/domain/services/ManagedModeDurability.test.js`:
- Around line 403-413: The test expectation is wrong: GraphRefManager.syncHead
returns updated: false when the current tip equals the newTipSha; update the
assertion in ManagedModeDurability.test.js (the it block calling
refManager.syncHead) to expect updated: false (keep anchor: false and sha:
'existingsha') so it matches the GraphRefManager.syncHead behavior.
🧹 Nitpick comments (4)
SEMANTICS.md (1)
60-74: Consider adding a language identifier to the pseudocode block.The static analysis tool flags this as a fenced code block without a language. For pseudocode, you could use
textorplaintextto satisfy linters while indicating it's not executable code.📝 Suggested fix
-``` +```text sync(ref, new_commit): if ref does not exist: set ref → new_commitsrc/domain/services/HealthCheckService.js (1)
131-149: Theif (!fromCache)check is redundant.When
cachedAtis truthy (line 134), the function returns early. When execution reaches line 139,cachedAtis alwaysnullandfromCacheis alwaysfalse(perCachedValue.getWithMetadatacontract for fresh computations). The condition!fromCachewill always be true at this point.Consider simplifying:
♻️ Proposed simplification
async getHealth() { const { value, cachedAt, fromCache } = await this._healthCache.getWithMetadata(); if (cachedAt) { return { ...value, cachedAt }; } // Log only for fresh computations - if (!fromCache) { - this._logger.debug('Health check completed', { - operation: 'getHealth', - status: value.status, - repositoryStatus: value.components.repository.status, - indexStatus: value.components.index.status, - }); - } + this._logger.debug('Health check completed', { + operation: 'getHealth', + status: value.status, + repositoryStatus: value.components.repository.status, + indexStatus: value.components.index.status, + }); return value; }src/domain/utils/CachedValue.js (1)
113-117:nullreturn values fromcompute()are not cached.The
_isValid()method returnsfalsewhen_value === null, which means ifcompute()legitimately returnsnull, it will be recomputed on everyget()call. The test suite documents this behavior (line 308-309), so if intentional, consider adding a JSDoc note to the class documentation.If caching
nullis needed in the future, consider using a separate_hasValueflag or a sentinel value.README.md (1)
1111-1116: Consider tightening “very large” wording.LanguageTool flags “very large” as a weak intensifier; consider a more precise term (e.g., “extremely large” or “at >10M nodes”).
Comment on lines
+10
to
+11
| ## [3.0.0] - 2025-01-30 | ||
|
|
There was a problem hiding this comment.
Fix release date to keep chronology consistent.
Line 10 lists 2025-01-30, which is out of sequence with the 2026 entries below.
✏️ Proposed fix
-## [3.0.0] - 2025-01-30
+## [3.0.0] - 2026-01-30📝 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
| ## [3.0.0] - 2025-01-30 | |
| ## [3.0.0] - 2026-01-30 |
🤖 Prompt for AI Agents
In `@CHANGELOG.md` around lines 10 - 11, Update the release date in the changelog
header "## [3.0.0] - 2025-01-30" so it is chronologically consistent with the
2026 entries below (for example change to "2026-01-30" or the correct 2026 date
you intend); edit the version header line only to reflect the correct year while
preserving the "## [3.0.0]" text.
Comment on lines
+9
to
+14
| # Copy alfred first (dependency) | ||
| COPY alfred ../alfred | ||
| # Copy empty-graph | ||
| COPY empty-graph/package*.json ./ | ||
| RUN npm install | ||
| COPY . . | ||
| COPY empty-graph . |
There was a problem hiding this comment.
CI build failure: alfred directory not found in build context.
The pipeline reports: COPY alfred ../alfred could not be performed. '/alfred' not found in build context.
The Dockerfile expects alfred to exist as a sibling directory when using the parent directory as build context. This works locally in a monorepo structure but fails in CI, likely because:
- CI only checks out the
empty-graphrepository, not the parent monorepo - The
alfredpackage isn't available in the CI build environment
Potential solutions:
- If using a monorepo: Ensure CI checks out the entire monorepo, not just
empty-graph - If separate repos: Publish
@git-stunts/alfredto npm and remove the local dependency - For CI-only workaround: Add a CI step to clone/copy alfred before the Docker build
#!/bin/bash
# Verify the expected directory structure and CI configuration
echo "=== Checking for monorepo indicators ==="
# Look for workspace configuration
cat package.json 2>/dev/null | jq '.workspaces // "no workspaces defined"'
# Check if there's a parent package.json that might define workspaces
if [ -f ../package.json ]; then
echo "=== Parent package.json found ==="
cat ../package.json | jq '.workspaces // "no workspaces"'
fi
# Check CI workflow files for context
echo "=== CI workflow configuration ==="
fd -e yml -e yaml . .github/workflows --exec cat {} \; 2>/dev/null | head -100🧰 Tools
🪛 GitHub Actions: CI
[error] 10-10: Docker build failed: COPY alfred ../alfred could not be performed. '/alfred' not found in build context.
🤖 Prompt for AI Agents
In `@Dockerfile` around lines 9 - 14, The Dockerfile's COPY alfred ../alfred step
fails in CI because the alfred directory isn't in the build context; update the
build to remove the local sibling dependency or ensure CI provides it: either
make CI check out the monorepo before docker build (so the Dockerfile's COPY
alfred ../alfred succeeds), or stop copying the local package and depend on the
published package (publish `@git-stunts/alfred` to npm and update package.json in
empty-graph), or add a CI step that clones/copies the alfred repo into the build
context prior to running the Dockerfile; locate the COPY alfred ../alfred and
related COPY empty-graph / RUN npm install lines in the Dockerfile and apply one
of these fixes accordingly.
Comment on lines
+52
to
+90
| ``` | ||
| +-------------------------------------------------------------+ | ||
| | EmptyGraph | <- Facade | ||
| | (index.js) | | ||
| +-------------------------------------------------------------+ | ||
| | Domain Services | | ||
| | +-------------+ +---------------+ +--------------------+ | | ||
| | | GraphService| | IndexRebuild | | TraversalService | | | ||
| | | | | Service | | | | | ||
| | +-------------+ +---------------+ +--------------------+ | | ||
| | +-------------+ +---------------+ +--------------------+ | | ||
| | | GraphRef | | BitmapIndex | | BitmapIndex | | | ||
| | | Manager | | Builder | | Reader | | | ||
| | +-------------+ +---------------+ +--------------------+ | | ||
| | +-------------+ +---------------+ +--------------------+ | | ||
| | | HealthCheck | | GitLogParser | | Streaming | | | ||
| | | Service | | | | BitmapIndexBuilder | | | ||
| | +-------------+ +---------------+ +--------------------+ | | ||
| +-------------------------------------------------------------+ | ||
| | Ports | | ||
| | +-------------------+ +---------------------------+ | | ||
| | | GraphPersistence | | IndexStoragePort | | | ||
| | | Port | | | | | ||
| | +-------------------+ +---------------------------+ | | ||
| | +-------------------+ +---------------------------+ | | ||
| | | LoggerPort | | ClockPort | | | ||
| | +-------------------+ +---------------------------+ | | ||
| +-------------------------------------------------------------+ | ||
| | Adapters | | ||
| | +-------------------+ +---------------------------+ | | ||
| | | GitGraphAdapter | | ConsoleLogger | | | ||
| | | | | NoOpLogger | | | ||
| | +-------------------+ +---------------------------+ | | ||
| | +-------------------+ | | ||
| | | PerformanceClock | | | ||
| | | GlobalClock | | | ||
| | +-------------------+ | | ||
| +-------------------------------------------------------------+ | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
markdownlint flags multiple blocks; apply language tags consistently (e.g., text, mermaid, json, javascript).
Example fix (apply similarly to other fences)
-```
+```text
...
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 73-73: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/ARCHITECTURE.md` around lines 52 - 90, The fenced diagram block (showing
EmptyGraph, Domain Services like GraphService/IndexRebuild/TraversalService,
Ports like GraphPersistence/IndexStoragePort, and Adapters like
GitGraphAdapter/ConsoleLogger) lacks a language identifier; update the opening
triple-backtick for this and all other fenced blocks in ARCHITECTURE.md to
include an appropriate language tag (e.g., text, mermaid, json, or javascript)
so markdownlint MD040 is satisfied—locate the block containing "EmptyGraph
(index.js)" and similar blocks and change ``` to ```text (or a more specific
language where appropriate) for each fence.
Comment on lines
+409
to
+426
| ## Performance Characteristics | ||
|
|
||
| | Operation | Complexity | Notes | | ||
| |-----------|------------|-------| | ||
| | Write (createNode) | O(1) | Append-only commit | | ||
| | Read (readNode) | O(1) | Direct SHA lookup | | ||
| | Unindexed traversal | O(N) | Linear scan via git log | | ||
| | Indexed lookup | O(1) | Bitmap query + ID resolution | | ||
| | Index rebuild | O(N) | One-time scan | | ||
| | Index load | O(1) initial | Lazy shard loading | | ||
|
|
||
| **Memory characteristics:** | ||
| | Scenario | Approximate Memory | | ||
| |----------|-------------------| | ||
| | Cold start (no index) | Near-zero | | ||
| | Single shard loaded | 0.5-2 MB per prefix | | ||
| | Full index (1M nodes) | 150-200 MB | | ||
|
|
There was a problem hiding this comment.
Fix table pipe spacing to satisfy MD060.
markdownlint flags compact table formatting in this section.
Example fix
-| Operation | Complexity | Notes |
-|-----------|------------|-------|
+| Operation | Complexity | Notes |
+| --------- | ---------- | ----- |🤖 Prompt for AI Agents
In `@docs/ARCHITECTURE.md` around lines 409 - 426, The markdown tables under the
"Performance Characteristics" section (headers and "Memory characteristics"
table) use compact pipe formatting that triggers MD060; update each table row
and header so there is a single space on both sides of every pipe (e.g., change
"|Write (createNode)|O(1)|Append-only commit|" to "| Write (createNode) | O(1) |
Append-only commit |") and ensure the separator row uses spaced pipes too (e.g.,
"| --- | --- | --- |"); apply this change to both tables in the same section so
markdownlint no longer flags MD060.
Comment on lines
+63
to
+84
| async _findDisconnectedTips() { | ||
| if (this._createdShas.length <= 1) { | ||
| return [...this._createdShas]; | ||
| } | ||
|
|
||
| const tips = []; | ||
| for (const candidate of this._createdShas) { | ||
| let isAncestorOfAnother = false; | ||
| for (const other of this._createdShas) { | ||
| if (candidate !== other) { | ||
| if (await this._graph._persistence.isAncestor(candidate, other)) { | ||
| isAncestorOfAnother = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (!isAncestorOfAnother) { | ||
| tips.push(candidate); | ||
| } | ||
| } | ||
| return tips; | ||
| } |
There was a problem hiding this comment.
Reduce nesting in _findDisconnectedTips to satisfy max-depth lint.
Line 73 hits the max-depth rule; flattening the loops avoids the lint failure and improves readability.
🧹 Proposed refactor
async _findDisconnectedTips() {
if (this._createdShas.length <= 1) {
return [...this._createdShas];
}
- const tips = [];
- for (const candidate of this._createdShas) {
- let isAncestorOfAnother = false;
- for (const other of this._createdShas) {
- if (candidate !== other) {
- if (await this._graph._persistence.isAncestor(candidate, other)) {
- isAncestorOfAnother = true;
- break;
- }
- }
- }
- if (!isAncestorOfAnother) {
- tips.push(candidate);
- }
- }
- return tips;
+ const tips = [];
+ const isAncestorOfAny = async candidate => {
+ for (const other of this._createdShas) {
+ if (candidate === other) { continue; }
+ if (await this._graph._persistence.isAncestor(candidate, other)) {
+ return true;
+ }
+ }
+ return false;
+ };
+
+ for (const candidate of this._createdShas) {
+ if (!(await isAncestorOfAny(candidate))) {
+ tips.push(candidate);
+ }
+ }
+ return tips;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async _findDisconnectedTips() { | |
| if (this._createdShas.length <= 1) { | |
| return [...this._createdShas]; | |
| } | |
| const tips = []; | |
| for (const candidate of this._createdShas) { | |
| let isAncestorOfAnother = false; | |
| for (const other of this._createdShas) { | |
| if (candidate !== other) { | |
| if (await this._graph._persistence.isAncestor(candidate, other)) { | |
| isAncestorOfAnother = true; | |
| break; | |
| } | |
| } | |
| } | |
| if (!isAncestorOfAnother) { | |
| tips.push(candidate); | |
| } | |
| } | |
| return tips; | |
| } | |
| async _findDisconnectedTips() { | |
| if (this._createdShas.length <= 1) { | |
| return [...this._createdShas]; | |
| } | |
| const tips = []; | |
| const isAncestorOfAny = async candidate => { | |
| for (const other of this._createdShas) { | |
| if (candidate === other) { continue; } | |
| if (await this._graph._persistence.isAncestor(candidate, other)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| }; | |
| for (const candidate of this._createdShas) { | |
| if (!(await isAncestorOfAny(candidate))) { | |
| tips.push(candidate); | |
| } | |
| } | |
| return tips; | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 73-73: ESLint: Blocks are nested too deeply (4). Maximum allowed is 3 (max-depth).
🪛 GitHub Check: lint
[failure] 73-73:
Blocks are nested too deeply (4). Maximum allowed is 3
🤖 Prompt for AI Agents
In `@index.js` around lines 63 - 84, The nested loops in _findDisconnectedTips
cause max-depth lint errors; extract the inner ancestor-check into a small async
helper (e.g., _isAncestorOfAnother(candidate)) that iterates this._createdShas
and returns true as soon as this._graph._persistence.isAncestor(candidate,
other) is true, then in _findDisconnectedTips replace the inner loop with: if
(!(await this._isAncestorOfAnother(candidate))) tips.push(candidate); this
flattens the control flow while keeping references to this._createdShas and
this._graph._persistence.isAncestor.
Comment on lines
+35
to
+41
| constructor({ persistence, logger = new NoOpLogger() }) { | ||
| if (!persistence) { | ||
| throw new Error('GraphRefManager requires a persistence adapter'); | ||
| } | ||
| this.persistence = persistence; | ||
| this.logger = logger; | ||
| } |
There was a problem hiding this comment.
Validate required persistence methods up front (including isAncestor).
Without checks, managed mode fails later with less actionable errors if a custom adapter is incomplete.
🛡️ Proposed guard
constructor({ persistence, logger = new NoOpLogger() }) {
if (!persistence) {
throw new Error('GraphRefManager requires a persistence adapter');
}
+ const required = ['readRef', 'updateRef', 'commitNode', 'isAncestor'];
+ for (const method of required) {
+ if (typeof persistence[method] !== 'function') {
+ throw new Error(`GraphRefManager requires persistence.${method}()`);
+ }
+ }
this.persistence = persistence;
this.logger = logger;
}📝 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
| constructor({ persistence, logger = new NoOpLogger() }) { | |
| if (!persistence) { | |
| throw new Error('GraphRefManager requires a persistence adapter'); | |
| } | |
| this.persistence = persistence; | |
| this.logger = logger; | |
| } | |
| constructor({ persistence, logger = new NoOpLogger() }) { | |
| if (!persistence) { | |
| throw new Error('GraphRefManager requires a persistence adapter'); | |
| } | |
| const required = ['readRef', 'updateRef', 'commitNode', 'isAncestor']; | |
| for (const method of required) { | |
| if (typeof persistence[method] !== 'function') { | |
| throw new Error(`GraphRefManager requires persistence.${method}()`); | |
| } | |
| } | |
| this.persistence = persistence; | |
| this.logger = logger; | |
| } |
🤖 Prompt for AI Agents
In `@src/domain/services/GraphRefManager.js` around lines 35 - 41, In the
GraphRefManager constructor validate that the provided persistence adapter
implements the required API (at minimum isAncestor) before assigning it; add a
short validation block in constructor({ persistence, logger = new NoOpLogger()
}) that defines an array of required method names (including "isAncestor") and
checks each exists and is a function on persistence, and if any are missing
throw a clear Error listing the missing method(s) so incomplete adapters fail
fast.
Comment on lines
+102
to
+152
| async syncHead(ref, newTipSha) { | ||
| const startTime = performance.now(); | ||
| const currentTip = await this.readHead(ref); | ||
|
|
||
| if (!currentTip) { | ||
| // No ref exists - create it | ||
| await this.persistence.updateRef(ref, newTipSha); | ||
| const durationMs = performance.now() - startTime; | ||
| this.logger.debug('Ref created', { | ||
| operation: 'syncHead', | ||
| ref, | ||
| sha: newTipSha, | ||
| anchor: false, | ||
| durationMs, | ||
| }); | ||
| return { updated: true, anchor: false, sha: newTipSha }; | ||
| } | ||
|
|
||
| if (currentTip === newTipSha) { | ||
| // Already pointing here | ||
| return { updated: false, anchor: false, sha: currentTip }; | ||
| } | ||
|
|
||
| // NEW: Fast-forward check - if current tip is ancestor of new commit, just move ref | ||
| if (await this.persistence.isAncestor(currentTip, newTipSha)) { | ||
| await this.persistence.updateRef(ref, newTipSha); | ||
| const durationMs = performance.now() - startTime; | ||
| this.logger.debug('Ref fast-forwarded', { | ||
| operation: 'syncHead', | ||
| ref, | ||
| sha: newTipSha, | ||
| previousTip: currentTip, | ||
| anchor: false, | ||
| durationMs, | ||
| }); | ||
| return { updated: true, anchor: false, sha: newTipSha }; | ||
| } | ||
|
|
||
| // Divergent history - create anchor | ||
| const anchorSha = await this.createAnchor([currentTip, newTipSha]); | ||
| await this.persistence.updateRef(ref, anchorSha); | ||
| const durationMs = performance.now() - startTime; | ||
| this.logger.debug('Anchor created', { | ||
| operation: 'syncHead', | ||
| ref, | ||
| sha: anchorSha, | ||
| parents: [currentTip, newTipSha], | ||
| anchor: true, | ||
| durationMs, | ||
| }); | ||
| return { updated: true, anchor: true, sha: anchorSha }; |
There was a problem hiding this comment.
syncHead exceeds max-lines lint; extract branch helpers.
Splitting the “create ref”, “fast‑forward”, and “anchor” branches into private methods should satisfy the rule and improve readability.
🧰 Tools
🪛 GitHub Check: lint
[failure] 102-102:
Async method 'syncHead' has too many lines (52). Maximum allowed is 50
🤖 Prompt for AI Agents
In `@src/domain/services/GraphRefManager.js` around lines 102 - 152, The syncHead
function is too long—extract the three main branches into private helpers: one
to handle the "no current tip / create ref" branch (call it e.g.
_handleCreateRef), one for the "fast-forward" branch (_handleFastForward) and
one for the "divergent history / anchor" branch (_handleCreateAnchor); each
helper should accept the inputs it needs (ref, newTipSha, currentTip, startTime
or compute duration inside) and perform the calls to this.persistence.updateRef,
this.logger.debug and return the same shape ({updated, anchor, sha}) so syncHead
becomes a thin coordinator that calls readHead, then delegates to
_handleCreateRef, _handleFastForward (after calling this.persistence.isAncestor)
or _handleCreateAnchor (which should call this.createAnchor and updateRef).
Ensure helpers preserve existing log keys (operation:'syncHead', ref, sha,
parents/previousTip, anchor, durationMs) and reuse methods readHead,
createAnchor, persistence.isAncestor, persistence.updateRef, and logger.debug.
Comment on lines
+383
to
+401
| /** | ||
| * Checks if one commit is an ancestor of another. | ||
| * Uses `git merge-base --is-ancestor` for efficient ancestry testing. | ||
| * | ||
| * @param {string} potentialAncestor - The commit that might be an ancestor | ||
| * @param {string} descendant - The commit that might be a descendant | ||
| * @returns {Promise<boolean>} True if potentialAncestor is an ancestor of descendant | ||
| */ | ||
| async isAncestor(potentialAncestor, descendant) { | ||
| this._validateOid(potentialAncestor); | ||
| this._validateOid(descendant); | ||
| try { | ||
| await this._executeWithRetry({ | ||
| args: ['merge-base', '--is-ancestor', potentialAncestor, descendant] | ||
| }); | ||
| return true; // Exit code 0 means it IS an ancestor | ||
| } catch { | ||
| return false; // Exit code 1 means it is NOT an ancestor | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the GitGraphAdapter.js file to find _executeWithRetry
cd src/infrastructure/adapters && wc -l GitGraphAdapter.jsRepository: git-stunts/empty-graph
Length of output: 88
🏁 Script executed:
# Read the full file to understand _executeWithRetry implementation
cat -n src/infrastructure/adapters/GitGraphAdapter.js | head -100Repository: git-stunts/empty-graph
Length of output: 3841
🏁 Script executed:
# Search for _executeWithRetry definition
rg -n "_executeWithRetry" src/infrastructure/adapters/GitGraphAdapter.js -A 20Repository: git-stunts/empty-graph
Length of output: 8622
🏁 Script executed:
# Look for any error handling patterns and exitCode usage in the file
rg -n "exitCode|exit.code" src/infrastructure/adapters/GitGraphAdapter.js -B 2 -A 2Repository: git-stunts/empty-graph
Length of output: 48
🏁 Script executed:
# Search for tests related to isAncestor
fd -t f "\.test\.|\.spec\." | xargs rg -l "isAncestor" 2>/dev/nullRepository: git-stunts/empty-graph
Length of output: 121
🏁 Script executed:
# Check test file for isAncestor or GitGraphAdapter tests
fd -t f "\.test\.|\.spec\." | xargs grep -l "GitGraphAdapter\|isAncestor" 2>/dev/nullRepository: git-stunts/empty-graph
Length of output: 276
🏁 Script executed:
# Look for plumbing package or how errors are thrown
rg -n "plumbing\.execute|throw.*Error" src/infrastructure/adapters/GitGraphAdapter.js -B 2 -A 3Repository: git-stunts/empty-graph
Length of output: 3682
🏁 Script executed:
# Check if exitCode is used anywhere in the codebase
rg -r "exitCode|exit.code" src/ test/ --type js | head -20Repository: git-stunts/empty-graph
Length of output: 2977
🏁 Script executed:
# Search for `@git-stunts/alfred` package info or documentation
find . -name "package.json" -o -name "package-lock.json" | xargs grep -l "alfred\|plumbing" 2>/dev/null | head -5Repository: git-stunts/empty-graph
Length of output: 100
🏁 Script executed:
# Look for any node_modules or installed packages info
cat package.json 2>/dev/null | head -50Repository: git-stunts/empty-graph
Length of output: 2367
🏁 Script executed:
# Look at the GitGraphAdapter test file to see how isAncestor is tested
cat -n test/unit/domain/services/GitGraphAdapter.test.js | grep -A 20 -B 5 "isAncestor"Repository: git-stunts/empty-graph
Length of output: 48
🏁 Script executed:
# Check the plumbing error structure - search for how errors are thrown in tests
rg -n "mock|stub|throw" test/unit/domain/services/GitGraphAdapter.test.js -A 3 -B 1 | head -80Repository: git-stunts/empty-graph
Length of output: 2978
🏁 Script executed:
# Search for readRef tests to see error handling pattern
rg -n "readRef" test/unit/domain/services/GitGraphAdapter.test.js -A 5 -B 2 | head -60Repository: git-stunts/empty-graph
Length of output: 48
🏁 Script executed:
# Search more broadly for test patterns related to error handling
rg -n "throws|rejects|toThrow|Error" test/unit/domain/services/GitGraphAdapter.test.js | head -40Repository: git-stunts/empty-graph
Length of output: 804
🏁 Script executed:
# Look for the readRef implementation and tests more carefully
cat -n test/unit/domain/services/GitGraphAdapter.test.js | tail -200Repository: git-stunts/empty-graph
Length of output: 7386
🏁 Script executed:
# Look for any mention of merge-base or isAncestor tests specifically
rg -n "merge-base|isAncestor|nodeExists|ping" test/unit/domain/services/GitGraphAdapter.test.js -A 10 -B 2Repository: git-stunts/empty-graph
Length of output: 4016
🏁 Script executed:
# Search for how plumbing package or execute function handles exit codes
rg -n "execute|exitCode|exit.code" package.json package-lock.json | head -30Repository: git-stunts/empty-graph
Length of output: 48
🏁 Script executed:
# Look for plumbing usage patterns to understand error structure
rg -n "plumbing\." src/infrastructure/adapters/GitGraphAdapter.js | head -20Repository: git-stunts/empty-graph
Length of output: 372
🏁 Script executed:
# Check if there are any integration tests or error handling examples
find test -name "*.test.js" -o -name "*.spec.js" | xargs rg -l "execute.*reject\|mockRejected" | head -3Repository: git-stunts/empty-graph
Length of output: 48
🏁 Script executed:
# Look for the readRef implementation more carefully to see error pattern
sed -n '262,282p' src/infrastructure/adapters/GitGraphAdapter.jsRepository: git-stunts/empty-graph
Length of output: 657
🏁 Script executed:
# Let's check if there's any documentation on the plumbing package error structure
cat package-lock.json | grep -A 10 "@git-stunts/plumbing"Repository: git-stunts/empty-graph
Length of output: 885
🏁 Script executed:
# Look for any error structure definition or usage in the codebase
rg -n "error\.|Error(" src/infrastructure/adapters/GitGraphAdapter.js | head -20Repository: git-stunts/empty-graph
Length of output: 144
🏁 Script executed:
# Check test expectations for error structure
rg -n "mockRejectedValue|rejects.toThrow" test/unit/domain/services/GitGraphAdapter.test.js -A 2 -B 1 | head -40Repository: git-stunts/empty-graph
Length of output: 1438
🏁 Script executed:
# Look at how readRef test errors are structured
grep -A 10 "readRef" test/unit/domain/services/GitGraphAdapter.test.jsRepository: git-stunts/empty-graph
Length of output: 48
🏁 Script executed:
# Let's look at how nodeExists works - it also swallows errors like isAncestor
sed -n '344,350p' src/infrastructure/adapters/GitGraphAdapter.jsRepository: git-stunts/empty-graph
Length of output: 209
🏁 Script executed:
# Let's see the ping method too, which also uses catch-all error handling
sed -n '357,367p' src/infrastructure/adapters/GitGraphAdapter.jsRepository: git-stunts/empty-graph
Length of output: 385
🏁 Script executed:
# Look for any documentation or type definitions for plumbing errors
find . -name "*.d.ts" -o -name "*.ts" | xargs grep -l "execute\|error" 2>/dev/null | head -5Repository: git-stunts/empty-graph
Length of output: 78
🏁 Script executed:
# Check if there are any TypeScript type definitions
fd "\.d\.ts$" | head -10Repository: git-stunts/empty-graph
Length of output: 76
🏁 Script executed:
# Let's search for any integration tests that might show actual error behavior
rg -n "isAncestor|nodeExists|merge-base" test/ -A 5 -B 2Repository: git-stunts/empty-graph
Length of output: 13990
🏁 Script executed:
# Search for any comments about exit codes in the codebase
rg -n "exit" src/infrastructure/adapters/GitGraphAdapter.js -B 2 -A 2Repository: git-stunts/empty-graph
Length of output: 48
🏁 Script executed:
# Check if there's any error handling with specific code/status properties
rg -n "\.code|\.status|\.exitCode" src/ --type js | head -20Repository: git-stunts/empty-graph
Length of output: 1625
🏁 Script executed:
# Let's look at the actual git documentation for merge-base exit codes
web_search
git merge-base --is-ancestor exit codes documentationRepository: git-stunts/empty-graph
Length of output: 163
🏁 Script executed:
# Check how err.code is used in BitmapIndexReader.js
rg -n "code:" src/ -B 3 -A 1 | head -30Repository: git-stunts/empty-graph
Length of output: 2203
🏁 Script executed:
# Search for how errors might have status or code properties from git commands
rg -n "err\.|error\." src/infrastructure/adapters/GitGraphAdapter.js -B 2 -A 2Repository: git-stunts/empty-graph
Length of output: 488
🌐 Web query:
git merge-base --is-ancestor exit code 1 2 documentation
💡 Result:
For git merge-base --is-ancestor <A> <B> the exit status is defined as:
- 0:
<A>is an ancestor of<B> - 1:
<A>is not an ancestor of<B> - Any other non-zero (e.g., 2, 128, …): an error occurred (invalid commit name, repository problem, etc.)—Git explicitly reserves “not ancestor” for status 1 and uses “not 1” for errors. [1]
Docs: Git’s git-merge-base manual, --is-ancestor option. [1]
Source(s):
[1] Git SCM: git-merge-base documentation (section --is-ancestor) (git-scm.com)
Citations:
🏁 Script executed:
# Let's check if there's any documentation in the repo about plumbing error handling
find . -name "README*" -o -name "*.md" | xargs grep -l "plumbing\|error\|exit" 2>/dev/null | head -5Repository: git-stunts/empty-graph
Length of output: 140
🏁 Script executed:
# Check if plumbing package is documented in node_modules (if available)
find . -path "*/node_modules/@git-stunts/plumbing*" -name "*.md" -o -name "*.d.ts" 2>/dev/null | head -5Repository: git-stunts/empty-graph
Length of output: 78
🏁 Script executed:
# Let's look at how other similar "bool returning" methods handle errors
sed -n '343,350p' src/infrastructure/adapters/GitGraphAdapter.js && echo "---" && sed -n '357,367p' src/infrastructure/adapters/GitGraphAdapter.jsRepository: git-stunts/empty-graph
Length of output: 561
🏁 Script executed:
# Check index.d.ts for type definitions about error handling
cat index.d.ts | head -100Repository: git-stunts/empty-graph
Length of output: 2335
isAncestor swallows all errors—must differentiate exit code 1 from actual failures.
Per git documentation, merge-base --is-ancestor exits with 0 (ancestor), 1 (not ancestor—expected outcome), or 2+ (error). The current catch-all returns false for all cases, masking repository/plumbing failures and leading to incorrect ancestry decisions. Tests document this as unimplemented (TODO).
Differentiate by checking if the error represents a non-ancestor result (exit 1) versus a real failure. Verify the actual plumbing error object structure to determine whether to check err.code, err.exitCode, or use message-based checks like readRef does elsewhere in this file.
🤖 Prompt for AI Agents
In `@src/infrastructure/adapters/GitGraphAdapter.js` around lines 383 - 401, The
isAncestor method currently catches all errors and returns false; change it to
only treat a non-ancestor git exit (exit code 1) as false and rethrow any other
errors so real failures aren't swallowed. Inside isAncestor (referencing
isAncestor, _executeWithRetry and _validateOid), catch the error object, inspect
its fields (e.g., err.code or err.exitCode and fallback to message parsing as
done in readRef elsewhere in this file) and if the code equals 1 return false,
otherwise throw the error so callers can handle plumbing/repo failures.
| import { createHash } from 'crypto'; | ||
| import BitmapIndexReader from '../../../../src/domain/services/BitmapIndexReader.js'; | ||
| import BitmapIndexBuilder from '../../../../src/domain/services/BitmapIndexBuilder.js'; | ||
| import BitmapIndexBuilder, { SHARD_VERSION } from '../../../../src/domain/services/BitmapIndexBuilder.js'; |
There was a problem hiding this comment.
Remove unused import SHARD_VERSION.
The static analysis tool reports that SHARD_VERSION is imported but never used in this file. Remove it to satisfy the linter.
🔧 Proposed fix
-import BitmapIndexBuilder, { SHARD_VERSION } from '../../../../src/domain/services/BitmapIndexBuilder.js';
+import BitmapIndexBuilder from '../../../../src/domain/services/BitmapIndexBuilder.js';📝 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
| import BitmapIndexBuilder, { SHARD_VERSION } from '../../../../src/domain/services/BitmapIndexBuilder.js'; | |
| import BitmapIndexBuilder from '../../../../src/domain/services/BitmapIndexBuilder.js'; |
🧰 Tools
🪛 GitHub Check: lint
[failure] 4-4:
'SHARD_VERSION' is defined but never used. Allowed unused vars must match /^_/u
🤖 Prompt for AI Agents
In `@test/unit/domain/services/BitmapIndexReader.test.js` at line 4, Remove the
unused named import SHARD_VERSION from the import statement that brings in
BitmapIndexBuilder (currently: import BitmapIndexBuilder, { SHARD_VERSION } from
...). Update the import to only import the default BitmapIndexBuilder (e.g.,
import BitmapIndexBuilder from ...), ensuring no other references to
SHARD_VERSION exist in BitmapIndexReader.test.js; run linter/tests to confirm
the unused-import warning is resolved.
Comment on lines
+403
to
+413
| it('syncHead returns same SHA when ref already points to it', async () => { | ||
| refState.tip = 'existingsha'; | ||
|
|
||
| const result = await refManager.syncHead('refs/empty-graph/test', 'existingsha'); | ||
|
|
||
| expect(result).toEqual({ | ||
| updated: true, | ||
| anchor: false, | ||
| sha: 'existingsha', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test expects updated: true but syncHead returns updated: false when ref already points to the target SHA.
Per the GraphRefManager.syncHead implementation (lines 117-118 in relevant snippet), when currentTip === newTipSha, it returns { updated: false, anchor: false, sha: currentTip }. The test expectation at line 409 should be updated: false.
🐛 Proposed fix
const result = await refManager.syncHead('refs/empty-graph/test', 'existingsha');
expect(result).toEqual({
- updated: true,
+ updated: false,
anchor: false,
sha: 'existingsha',
});📝 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('syncHead returns same SHA when ref already points to it', async () => { | |
| refState.tip = 'existingsha'; | |
| const result = await refManager.syncHead('refs/empty-graph/test', 'existingsha'); | |
| expect(result).toEqual({ | |
| updated: true, | |
| anchor: false, | |
| sha: 'existingsha', | |
| }); | |
| }); | |
| it('syncHead returns same SHA when ref already points to it', async () => { | |
| refState.tip = 'existingsha'; | |
| const result = await refManager.syncHead('refs/empty-graph/test', 'existingsha'); | |
| expect(result).toEqual({ | |
| updated: false, | |
| anchor: false, | |
| sha: 'existingsha', | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In `@test/unit/domain/services/ManagedModeDurability.test.js` around lines 403 -
413, The test expectation is wrong: GraphRefManager.syncHead returns updated:
false when the current tip equals the newTipSha; update the assertion in
ManagedModeDurability.test.js (the it block calling refManager.syncHead) to
expect updated: false (keep anchor: false and sha: 'existingsha') so it matches
the GraphRefManager.syncHead behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes a critical bug! Forgot about git gc. Now it is GC-proof.
Summary by CodeRabbit
New Features
Performance
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.