Package

[flyingrobots]

Summary by CodeRabbit

• New Features

  • Public EmptyGraph library: create/read/list/iterate nodes, index rebuild/load/save, and parent/child lookups; benchmark runner and interactive results page.

• Documentation

  • Added comprehensive docs: README, architecture, security, audits, changelog, contributing, code of conduct, notices, tasklist, and supporting material.

• Tests

  • Extensive unit and benchmark suites covering nodes, indexing, rebuilds, serialization, and query workflows.

• Chores

  • CI workflow, Docker/Docker Compose configs, linting config, package metadata, ignore files, and TypeScript declarations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Adds a Git-backed EmptyGraph library with immutable GraphNode, streaming GraphService, sharded Roaring-bitmap index (BitmapIndexService) and rebuild/load service (CacheRebuildService), a Git persistence port and GitGraphAdapter, an EmptyGraph facade with typings, CI/Docker tooling, benchmarks, docs, and tests.

Changes

Cohort / File(s)	Summary
Repo metadata & docs \.dockerignore, \.gitignore, package.json, NOTICE, CHANGELOG.md, CODE_OF_CONDUCT.md, CONTRIBUTING.md, SECURITY.md, AUDITS.md, TASKLIST.md	Add ignore rules, package manifest, license/notice, changelog, contributor/conduct/security/audit docs, and project task tracking.
CI / Docker / linting \.github/workflows/ci.yml, Dockerfile, Dockerfile.benchmark, docker-compose.yml, eslint.config.js	Add GitHub Actions CI (lint + docker test), Dockerfiles for test/benchmark, docker‑compose services, and ESLint config with overrides for tests/benchmarks.
High-level docs & marketing README.md, ARCHITECTURE.md, GIT_STUNTS_MATERIAL.md, THE_STUNT.md, benchmarks/index.html	Add README, architecture/design docs, blog/stunt material, technical stunt writeup, and interactive benchmark visualization.
Public facade & typings index.js, index.d.ts	Add EmptyGraph facade (default export) with named exports for services/adapters/entities, DEFAULT_INDEX_REF, and full TypeScript declarations for the public API.
Domain: entities & services src/domain/entities/GraphNode.js, src/domain/services/GraphService.js, src/domain/services/BitmapIndexService.js, src/domain/services/CacheRebuildService.js	Add immutable GraphNode entity; GraphService with RECORD_SEPARATOR streaming/parsing; BitmapIndexService (sharded Roaring bitmap index, lazy shard loading, lookupId, getParents/getChildren, rebuild/serialize helpers); CacheRebuildService to build and persist index trees.
Persistence port & adapter src/ports/GraphPersistencePort.js, src/infrastructure/adapters/GitGraphAdapter.js	Add abstract GraphPersistencePort interface and GitGraphAdapter implementing git plumbing operations (commit/show/log, blob/tree IO, refs) with input validation and stream helpers.
Benchmarks & results benchmarks/benchmark.js, benchmarks/results.json, benchmarks/index.html, Dockerfile.benchmark	Add multi-scale benchmark runner, results dataset, static visualization, and benchmark Dockerfile.
Tests — unit & bench test/benchmark/graph.bench.js, test/unit/domain/entities/GraphNode.test.js, test/unit/domain/services/GraphService.test.js, test/unit/domain/services/BitmapIndexService.test.js, test/unit/domain/services/CacheRebuildService.test.js	Add Vitest benchmarks and unit tests covering GraphNode validation/immutability, GraphService parsing/delegation, BitmapIndexService rebuild/serialize/lookup behavior, and CacheRebuildService persistence interactions.

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant EmptyGraph as EmptyGraph
    participant CacheRebuild as CacheRebuildService
    participant GraphSvc as GraphService
    participant BitmapIdx as BitmapIndexService
    participant GitAdapter as GitGraphAdapter

    User->>EmptyGraph: rebuildIndex(ref)
    EmptyGraph->>CacheRebuild: rebuild(ref)
    CacheRebuild->>BitmapIdx: createRebuildState()
    BitmapIdx-->>CacheRebuild: state
    CacheRebuild->>GraphSvc: iterateNodes({ref, limit})
    loop per node
        GraphSvc-->>CacheRebuild: GraphNode
        CacheRebuild->>BitmapIdx: _getOrCreateId(node.sha, state)
        BitmapIdx-->>CacheRebuild: id
        loop per parent
            CacheRebuild->>BitmapIdx: addEdge(parentSha, node.sha, state)
        end
    end
    CacheRebuild->>BitmapIdx: serialize(state)
    BitmapIdx-->>CacheRebuild: {blobs, entries}
    loop per blob
        CacheRebuild->>GitAdapter: writeBlob(buffer)
        GitAdapter-->>CacheRebuild: oid
    end
    CacheRebuild->>GitAdapter: writeTree(entries)
    GitAdapter-->>CacheRebuild: treeOid
    CacheRebuild-->>EmptyGraph: treeOid
    EmptyGraph-->>User: treeOid

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hop through commits and nibble at the tree,

Bitmaps hum in patches, mapping sha to me.
I stitch small blobs and write a tidy tree,
Lookups dart like rabbits—fast, light, and free.
Hooray for the Empty Graph jubilee.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title 'Package' is too vague and generic, failing to describe the actual scope of changes which include comprehensive initial project setup, documentation, CI/CD configuration, core library implementation, and comprehensive test coverage.	Replace the title with a descriptive summary of the main changes, such as 'Initial package setup with EmptyGraph library implementation, CI/CD workflows, and comprehensive documentation' or 'Add EmptyGraph library with bitmap indexing, tests, and project infrastructure'.
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 10

🤖 Fix all issues with AI agents

In @.github/workflows/ci.yml:
- Around line 16-18: The CI uses Node.js 20 while Docker uses Node.js 22; update
the node-version setting in the GitHub Actions workflow for both the lint and
test jobs by changing the with: node-version value used in the jobs named "lint"
and "test" to '22' so the runner environment matches the Dockerfile and ensures
consistent linting and test behavior.

In `@Dockerfile.benchmark`:
- Around line 7-9: The Dockerfile contains COPY instructions "COPY plumbing
packages/plumbing" and "COPY empty-graph packages/empty-graph" that reference
directories not present in the repo and will break docker build; update these
COPY paths in the Dockerfile to point to actual directories in this repository
(or remove them) or ensure the build context includes those external folders;
specifically, either replace the two COPY lines with correct source paths that
exist in the repo (or omit them if not needed), or document/configure the build
to be run from the parent monorepo so that "plumbing" and "empty-graph" exist in
the context, or add instructions to include those folders via docker build
context or .dockerignore changes.

In `@index.js`:
- Around line 24-26: The JSDoc `@param` import for options.plumbing uses an
incorrect relative path; update the type import in the JSDoc above the function
(the `@param` {import('../plumbing/index.js').default} options.plumbing) to point
to the local sibling module (use ./plumbing/index.js) so the type resolver can
find the plumbing default export.

In `@SECURITY.md`:
- Line 30: Update the sentence "If you discover a security vulnerability, please
send an e-mail to james@flyingrobots.dev." to use a markdown mailto link instead
of a bare email address; modify that line so the email is formatted like a
markdown link ([james@flyingrobots.dev](mailto:james@flyingrobots.dev)) to
satisfy MD034 and maintain consistency in SECURITY.md.

In `@src/domain/entities/GraphNode.js`:
- Around line 29-30: The constructor of GraphNode assigns this.parents directly
then calls Object.freeze(this), which is shallow and allows mutation of the
parents array; change the constructor to defensively copy and freeze the parents
array (e.g., create a new array from the incoming parents and call Object.freeze
on that) and assign that frozen array to this.parents before calling
Object.freeze(this) so external callers cannot mutate the parents contents;
reference the GraphNode constructor and the this.parents assignment when making
the change.

In `@src/domain/services/BitmapIndexService.js`:
- Around line 65-66: Replace the single-line if statements with block-braced
forms to satisfy lint rules: change occurrences like "if (!encoded) return [];"
to use braces and a block body (e.g., "if (!encoded) { return []; }"), and do
the same for the other offending single-line ifs mentioned (the blocks around
the conditions at the spots corresponding to lines 83-84 and 107-110) so each
conditional uses { } even for a single statement; update the statements in the
same functions/methods where "encoded" and the other checks occur to preserve
existing logic.

In `@src/domain/services/CacheRebuildService.js`:
- Around line 15-20: Avoid calling the private BitmapIndexService._getOrCreateId
from CacheRebuildService; add a public wrapper method (e.g.,
BitmapIndexService.registerNode(sha, state)) that internally delegates to
_getOrCreateId, then replace the direct call in the iterateNodes loop with
BitmapIndexService.registerNode(node.sha, state); keep
BitmapIndexService.addEdge(parentSha, node.sha, state) unchanged so nodes with
parents are still linked while isolated nodes get registered via the new public
API.

In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 49-66: The _validateRef method is rejecting valid git revspecs;
update its validation to allow dots in ref names and proper revspec operators
while keeping the leading '-' guard and length check. Replace the strict
validRefPattern with a relaxed pattern that permits dots in names (e.g.,
[a-zA-Z0-9._/-]+), allows optional ancestry syntax like (^|~) followed by
optional digits (e.g., (~\d*|^\d*)), and supports range/ancestor operators like
'..' between names (e.g., name(\.\.name)?); retain the existing checks for
non-empty string, max length 1024, and the startsWith('-' or '--') injection
prevention in _validateRef to ensure security.

In `@test/benchmark/graph.bench.js`:
- Around line 5-10: The benchmark instantiates GraphService without the required
persistence parameter so this leaves this.persistence undefined; update the
benchmark to pass a minimal mock persistence object into new GraphService({
persistence: ... }) (e.g., an object with the methods used by GraphService) so
the constructor signature constructor({ persistence }) is satisfied and future
operation tests won't throw; locate the instantiation in the bench('service
initialization', ...) block and replace new GraphService() with new
GraphService({ persistence: mockPersistence }) or similar.

In `@test/unit/domain/services/BitmapIndexService.test.js`:
- Around line 174-184: The test uses a shard key with the wrong extension; in
BitmapIndexService.test.js update the test data in the 'initializes shard OID
mappings from object' case so the shard filename uses the .json extension
(change 'shards_fwd_aa.bitmap' to 'shards_fwd_aa.json') to match the
implementation used by BitmapIndexService.setup and the serialize-related
expectations.

🧹 Nitpick comments (10)

eslint.config.js (1)

9-15: Consider adding additional Node.js globals.

The globals list may be incomplete for a Node.js project. Common additions include URL, URLSearchParams, TextEncoder, TextDecoder, setInterval, clearInterval, and __dirname/__filename (if using CommonJS anywhere).

However, if the codebase exclusively uses ES modules and doesn't reference these, the current set is sufficient.

Example expanded globals

       globals: {
         process: "readonly",
         Buffer: "readonly",
         console: "readonly",
         setTimeout: "readonly",
-        clearTimeout: "readonly"
+        clearTimeout: "readonly",
+        setInterval: "readonly",
+        clearInterval: "readonly",
+        URL: "readonly",
+        TextEncoder: "readonly",
+        TextDecoder: "readonly"
       }

.github/workflows/ci.yml (1)

31-33: Consider using docker compose (v2) instead of docker-compose.

The docker-compose command is the legacy v1 syntax. GitHub-hosted runners support Docker Compose v2 via docker compose (space-separated).

Additionally, npm install on line 31 may be redundant since tests run inside Docker, which performs its own dependency installation.

♻️ Suggested changes

       - run: npm install
       - name: Run tests in Docker
-        run: docker-compose run --rm test
+        run: docker compose run --rm test

You could also remove npm install from the test job if it's only needed for lint.

test/unit/domain/services/CacheRebuildService.test.js (1)

30-36: Consider strengthening assertions and adding edge case coverage.

The test verifies that persistence methods are called but doesn't assert on the arguments passed (e.g., blob content, tree entries format). Additionally, the load() method and edge cases (empty graph, single node) lack coverage.

💡 Example: Assert on writeBlob call count

     const treeOid = await service.rebuild('main');

-    expect(mockPersistence.writeBlob).toHaveBeenCalled();
+    // Expect one blob per shard (at minimum, verify call count)
+    expect(mockPersistence.writeBlob).toHaveBeenCalled();
+    expect(mockPersistence.writeBlob.mock.calls.length).toBeGreaterThan(0);
     expect(mockPersistence.writeTree).toHaveBeenCalled();
     expect(treeOid).toBe('tree-oid');

THE_STUNT.md (1)

38-48: Add benchmark environment/methodology notes for reproducibility.
A short note on hardware/OS/Node version and run methodology will help readers interpret and compare the timings safely.

test/unit/domain/services/BitmapIndexService.test.js (1)

520-525: Reminder: Uncomment assertions once query methods are implemented.

These commented assertions should be enabled after getParents and getChildren are implemented to complete the end-to-end validation.

Would you like me to open an issue to track uncommenting these assertions?

README.md (1)

179-198: Add language specifier to the fenced code block.

The architecture diagram should have a language specifier for the fenced code block. Since this is ASCII art, use text or plaintext to satisfy linting and improve rendering consistency.

♻️ Proposed fix

-```
+```text
 ┌─────────────────────────────────────────────┐
 │         EmptyGraph (Facade)                 │

benchmarks/index.html (2)

74-86: Consider loading benchmark data from results.json instead of hardcoding.

The AI summary indicates benchmarks/results.json exists with this data. Loading dynamically would avoid duplication and ensure the visualization stays in sync with benchmark runs.

Also, the inflectionX = 155172 magic number could benefit from a comment explaining how it was derived (the payoff point where index lookup beats legacy scan).

♻️ Example: Dynamic data loading

// Instead of hardcoded data:
fetch('./results.json')
  .then(res => res.json())
  .then(data => {
    // Use data for chart
  });

// Document the inflection point calculation
const inflectionX = 155172; // Node count where loadTimeMs + lookupTimeMs < scanTimeMs

---

7-9: Consider adding SRI hashes to external CDN scripts for supply chain security.

Loading scripts from CDNs without Subresource Integrity (SRI) hashes exposes the page to supply chain attacks. While d3js.org doesn't publish pre-computed hashes, you can generate them using online tools like srihash.org or the command line:

curl -s https://d3js.org/d3.v7.min.js | openssl dgst -sha384 -binary | openssl base64 -A

Then add the resulting hash to your script tags with crossorigin="anonymous":

<script src="https://d3js.org/d3.v7.min.js" integrity="sha384-<computed-hash>" crossorigin="anonymous"></script>

Repeat for the GSAP and MotionPathPlugin scripts from cdnjs. This is optional but recommended for production deployments.

src/domain/services/GraphService.js (1)

80-91: Potential bug: Empty parent line produces [''] instead of [].

When a commit has no parents (root commit), git log outputs an empty line for %P. The current code at line 87:

const parents = lines[3] ? lines[3].split(' ') : [];

An empty string '' is falsy, so this should work. However, if there's whitespace or a single space, ' '.split(' ') produces ['', '']. Consider using .filter(Boolean) for robustness:

♻️ Proposed fix

-    const parents = lines[3] ? lines[3].split(' ') : [];
+    const parents = lines[3] ? lines[3].split(' ').filter(Boolean) : [];

src/domain/services/CacheRebuildService.js (1)

12-30: Consider making the rebuild limit configurable.

The rebuild method hardcodes limit: 1000000. For very large graphs exceeding 1M nodes, this would miss nodes. Consider making it a parameter with a sensible default:

♻️ Proposed fix

-  async rebuild(ref) {
+  async rebuild(ref, { limit = 10_000_000 } = {}) {
     const state = BitmapIndexService.createRebuildState();
     
-    for await (const node of this.graphService.iterateNodes({ ref, limit: 1000000 })) {
+    for await (const node of this.graphService.iterateNodes({ ref, limit })) {

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 28-30: Input OIDs (sha, treeOid, oid) are not validated and can
start with '-' or contain non-hex characters causing git to treat them as
options; add a validator function (e.g., validateOid or isValidOid) that
enforces a strict hex regexp (e.g., /^[0-9a-fA-F]{40}$/) and call it at the
start of methods that accept OIDs (for example showNode(sha), the methods
referencing treeOid, and the methods using oid) to throw or return an error
before invoking this.plumbing.execute; ensure you apply the same check to all
places flagged (lines ~28-30, 91-94, 109-112) so no unvalidated input reaches
the git call.
- Around line 32-35: The logNodes method currently interpolates format into the
args unconditionally causing git errors when format is undefined; update async
logNodes({ ref, limit = 50, format }) to build the args array explicitly (e.g.,
['log', `-${limit}`]), and only push `--format=${format}` when format is
provided (or alternatively set a safe default format value) before calling
this.plumbing.execute({ args }). Keep the existing this._validateRef(ref) call
and ensure the plumbing.execute invocation uses the resulting args array.

♻️ Duplicate comments (1)

src/infrastructure/adapters/GitGraphAdapter.js (1)

49-66: Ref validation still rejects dotted/range revspecs.
The current regex blocks valid refs like feature.x, HEAD~2, and main..feature.

✅ Suggested fix (same as prior review)

-    const validRefPattern = /^[a-zA-Z0-9_/-]+(\^|~|\.\.|\.)*$/;
+    const rev = '[A-Za-z0-9][A-Za-z0-9._/-]*(?:[~^]\\d*)?';
+    const validRefPattern = new RegExp(`^${rev}(?:\\.\\.${rev})?$`);

🧹 Nitpick comments (5)

eslint.config.js (2)

9-17: Consider using the globals package for Node.js environment.

Manually defining globals works, but the list is incomplete (missing setInterval, clearInterval, URL, globalThis, etc.). Using the globals package provides comprehensive, maintained sets.

♻️ Optional: Use globals package

 import js from "@eslint/js";
+import globals from "globals";
 
 export default [
   js.configs.recommended,
   {
     languageOptions: {
       ecmaVersion: 2022,
       sourceType: "module",
-      globals: {
-        process: "readonly",
-        Buffer: "readonly",
-        console: "readonly",
-        setTimeout: "readonly",
-        clearTimeout: "readonly",
-        TextDecoder: "readonly",
-        TextEncoder: "readonly"
-      }
+      globals: {
+        ...globals.node
+      }
     },

Install with: npm install -D globals

---

48-48: Redundant file pattern.

test/**/*.test.js is already matched by test/**/*.js, so the second pattern is unnecessary.

♻️ Suggested fix

-    files: ["test/**/*.js", "test/**/*.test.js"],
+    files: ["test/**/*.js"],

README.md (2)

142-142: Clarify regex pattern documentation.

The regex pattern shows \~ (escaped tilde) in the documentation. In JavaScript regex, the tilde doesn't need escaping. Verify whether this should be documented as ~ or if the backslash is intentional for clarity.

📝 Suggested clarification

-- `ref` must match: `/^[a-zA-Z0-9_/-]+(\^|\~|\.\.|\.)*$/`
+- `ref` must match: `/^[a-zA-Z0-9_/-]+(\^|~|\.\.|\.)*$/`

Or if the backslash is for documentation clarity, consider adding a note:

-- `ref` must match: `/^[a-zA-Z0-9_/-]+(\^|\~|\.\.|\.)*$/`
+- `ref` must match: `/^[a-zA-Z0-9_/-]+(\^|~|\.\.|\.)*$/` (backslash shown for clarity only)

---

266-266: Add language identifier to fenced code block.

The architecture diagram is missing a language identifier. Consider specifying text or plaintext to satisfy the markdownlint rule and improve clarity.

Based on static analysis hints (markdownlint-cli2).

📝 Suggested fix

-```
+```text
 ┌─────────────────────────────────────────────┐
 │         EmptyGraph (Facade)                 │

src/infrastructure/adapters/GitGraphAdapter.js (1)

82-89: Consider bounding concurrency when reading many blobs.
Promise.all over a large tree can spike memory and open many simultaneous reads. A bounded or sequential read is safer for large repos.

♻️ Option: sequential read

  async readTree(treeOid) {
     const oids = await this.readTreeOids(treeOid);
     const files = {};
-    await Promise.all(Object.entries(oids).map(async ([path, oid]) => {
-      files[path] = await this.readBlob(oid);
-    }));
+    for (const [path, oid] of Object.entries(oids)) {
+      files[path] = await this.readBlob(oid);
+    }
     return files;
  }

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@index.js`:
- Around line 19-20: DEFAULT_INDEX_REF is declared but not exported at runtime
while index.d.ts exports it; export the constant so the runtime matches the
TypeScript declaration: make DEFAULT_INDEX_REF a named export (or attach it to
module.exports) so the symbol DEFAULT_INDEX_REF is available to consumers;
update the export mechanism that other exports use in this file (referencing
DEFAULT_INDEX_REF) to expose it.

In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 122-127: The updateRef method validates ref but not oid, allowing
an oid starting with '-' to be interpreted as a git option; add an oid
validation in updateRef (or a helper _validateOid) to ensure oid is a string,
does not start with '-' and matches an expected pattern (e.g. only hex chars and
reasonable length like /^[0-9a-fA-F]{4,40}$/ or a specific allowed token), and
throw an error on invalid input before calling this.plumbing.execute({ args:
['update-ref', ref, oid] }); reference the updateRef method and plumbing.execute
call when making the change.
- Around line 20-26: The commitNode method is vulnerable because entries in
parents are passed as git args without validation; add and use the existing
_validateOid helper to ensure each parent SHA is a valid hex OID (reject values
starting with '-' or non-hex) before building parentArgs, and throw or return an
error for invalid OIDs; update commitNode to call this._validateOid(p) for each
p in parents and only include validated values when producing parentArgs.

♻️ Duplicate comments (5)

src/infrastructure/adapters/GitGraphAdapter.js (4)

28-30: OID validation still missing for showNode.

As flagged in a prior review, the sha parameter is passed directly to git without validation, allowing potential option injection if it starts with -.

---

32-40: Guard against undefined format still needed.

As flagged in a prior review, interpolating format directly produces --format=undefined when not provided, causing a git error.

---

49-66: Ref validation regex remains too restrictive.

As flagged in a prior review, the pattern /^[a-zA-Z0-9_/-]+(\^|~|\.\.|\.)*$/ rejects valid git revspecs like feature.x, HEAD~2, and main..feature.

---

91-114: OID validation still missing for readTreeOids and readBlob.

As flagged in a prior review, these methods pass OIDs directly to git without validation.

index.js (1)

27-29: JSDoc type import path issue persists.

As flagged in a prior review, the JSDoc import path import('../plumbing/index.js') is incorrect for a file at the repository root.

🧹 Nitpick comments (2)

README.md (1)

309-328: Add language specifier to fenced code block.

The ASCII architecture diagram code block is missing a language specifier. For linting compliance, add a language identifier (e.g., text or plaintext).

📝 Proposed fix

-```
+```text
 ┌─────────────────────────────────────────────┐
 │         EmptyGraph (Facade)                 │

index.d.ts (1)

157-158: Consider removing private static method from public declarations.

_getOrCreateId is prefixed with underscore (indicating internal/private usage) but is exposed in the public type declarations. If this method is not intended for external consumption, consider removing it from the .d.ts file.

📝 Proposed fix

   /** Get or create a numeric ID for a SHA */
-  static _getOrCreateId(sha: string, state: BitmapRebuildState): number;

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/domain/services/GraphService.js`:
- Around line 66-88: The TextDecoder in _parseNodeStream currently calls
decoder.decode(chunk) which finalizes and can corrupt multibyte UTF-8 across
chunk boundaries; switch to streaming decode by using decoder.decode(chunk, {
stream: true }) when appending non-string chunks and after the for-await loop
call decoder.decode() with no args to flush remaining bytes into the buffer
before parsing the final block; update the code paths that set buffer (inside
the for-await loop and before the final _parseNode(buffer) call) to use these
streaming/flushed decode results so commit messages keep intact UTF-8
characters.

♻️ Duplicate comments (3)

src/infrastructure/adapters/GitGraphAdapter.js (2)

20-30: Validate OIDs before passing them to git.

Unvalidated sha, treeOid, oid, and parent SHAs can be interpreted as options or malformed inputs by git. Add a strict OID validator and apply it consistently.

✅ Proposed fix

   async commitNode({ message, parents = [], sign = false }) {
+    for (const p of parents) {
+      this._validateOid(p);
+    }
     const parentArgs = parents.flatMap(p => ['-p', p]);
     const signArgs = sign ? ['-S'] : [];
     const args = ['commit-tree', this.emptyTree, ...parentArgs, ...signArgs, '-m', message];

     return await this.plumbing.execute({ args });
   }

   async showNode(sha) {
+    this._validateOid(sha);
     return await this.plumbing.execute({ args: ['show', '-s', '--format=%B', sha] });
   }
@@
   async readTreeOids(treeOid) {
+    this._validateOid(treeOid);
     const output = await this.plumbing.execute({
       args: ['ls-tree', '-r', treeOid]
     });
@@
   async readBlob(oid) {
+    this._validateOid(oid);
     const stream = await this.plumbing.executeStream({
       args: ['cat-file', 'blob', oid]
     });
     return await stream.collect({ asString: false });
   }
@@
   async updateRef(ref, oid) {
     this._validateRef(ref);
+    this._validateOid(oid);
     await this.plumbing.execute({
       args: ['update-ref', ref, oid]
     });
   }
+
+  _validateOid(oid) {
+    if (!oid || typeof oid !== 'string') {
+      throw new Error('OID must be a non-empty string');
+    }
+    if (oid.length > 64) {
+      throw new Error(`OID too long: ${oid.length} chars. Maximum is 64`);
+    }
+    const validOidPattern = /^[0-9a-fA-F]{4,64}$/;
+    if (!validOidPattern.test(oid)) {
+      throw new Error(`Invalid OID format: ${oid}`);
+    }
+  }

Also applies to: 84-114, 124-128

---

32-40: Guard against --format=undefined in log methods.

If format is omitted, git treats --format=undefined as invalid. Build args conditionally or set a default format.

✅ Proposed fix

   async logNodes({ ref, limit = 50, format }) {
     this._validateRef(ref);
-    return await this.plumbing.execute({ args: ['log', `-${limit}`, `--format=${format}`, ref] });
+    const args = ['log', `-${limit}`];
+    if (format) args.push(`--format=${format}`);
+    args.push(ref);
+    return await this.plumbing.execute({ args });
   }

   async logNodesStream({ ref, limit = 1000000, format }) {
     this._validateRef(ref);
-    return await this.plumbing.executeStream({ args: ['log', `-${limit}`, `--format=${format}`, ref] });
+    const args = ['log', `-${limit}`];
+    if (format) args.push(`--format=${format}`);
+    args.push(ref);
+    return await this.plumbing.executeStream({ args });
   }

index.js (1)

19-20: DEFAULT_INDEX_REF should be exported to match TypeScript declarations.

The constant is declared as a local const but index.d.ts exports it. TypeScript consumers will see a type that doesn't exist at runtime.

Proposed fix

 /** Default ref for storing the index OID */
-const DEFAULT_INDEX_REF = 'refs/empty-graph/index';
+export const DEFAULT_INDEX_REF = 'refs/empty-graph/index';

🧹 Nitpick comments (4)

test/benchmark/graph.bench.js (1)

177-182: Consider using the public registerNode method for consistency.

Line 181 directly calls the private _getOrCreateId method. While acceptable in benchmark code, using BitmapIndexService.registerNode(sha, state) would be more consistent with the public API and align with the pattern used in CacheRebuildService.

     for (let i = 0; i < 10000; i++) {
       const sha = i.toString(16).padStart(40, '0');
-      BitmapIndexService._getOrCreateId(sha, state);
+      BitmapIndexService.registerNode(sha, state);
     }

test/unit/domain/services/BitmapIndexService.test.js (1)

520-526: Uncomment the end-to-end assertions for getParents/getChildren.

The comment states these "will fail until getParents/getChildren are implemented," but these methods are now fully implemented in BitmapIndexService.js. The assertions should be enabled to verify the full workflow.

Suggested fix

       expect(idB).toBe(1);

-      // These will fail until getParents/getChildren are implemented
-      // const parentsOfB = await service.getParents(SHA_B);
-      // expect(parentsOfB).toContain(SHA_A);
-      //
-      // const childrenOfA = await service.getChildren(SHA_A);
-      // expect(childrenOfA).toContain(SHA_B);
+      // Verify parent/child relationships
+      const parentsOfB = await service.getParents(SHA_B);
+      expect(parentsOfB).toContain(SHA_A);
+
+      const childrenOfA = await service.getChildren(SHA_A);
+      expect(childrenOfA).toContain(SHA_B);
     });

src/domain/services/BitmapIndexService.js (1)

127-130: Consider logging shard load failures for debugging.

The graceful degradation is appropriate for production resilience, but silently swallowing errors can make debugging difficult. Consider adding optional debug logging.

     } catch {
       // Graceful degradation: return empty shard on load failure
+      // Note: Consider adding debug logging here for troubleshooting
       return format === 'json' ? {} : new RoaringBitmap32();
     }

index.js (1)

103-107: Clarify that rebuildIndex does not automatically load the index.

After rebuildIndex, hasIndex returns false and getParents/getChildren will throw. This is intentional (allows saving without loading), but the JSDoc example could be clearer about the required follow-up call.

Consider updating the example:

    * `@example`
    * const treeOid = await graph.rebuildIndex('HEAD');
+   * await graph.loadIndex(treeOid); // Required before querying

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 98-105: The current readTree implementation uses Promise.all
across all entries which can spawn thousands of concurrent readBlob calls;
modify readTree (which calls readTreeOids and readBlob) to avoid unbounded
concurrency by iterating entries sequentially (e.g., for...of) or by
implementing a small bounded worker pool (limit concurrent readBlob invocations
to a safe number like 10–50 using a simple queue or a p-limit style helper) so
files[path] is filled as blobs complete without exhausting resources.
- Around line 36-54: Add a new validator method named _validateLimit that
mirrors the style of existing validators (_validateRef/_validateOid): ensure the
passed limit is a finite positive integer within an acceptable range (reject
non-numeric, non-integer, <=0, or excessively large values) and throw a clear
error if invalid. Call this._validateLimit(limit) at the start of both logNodes
and logNodesStream before building args so the limit is always validated
regardless of caller. Ensure error messages and thrown error types are
consistent with the existing validators' behavior.
- Around line 107-124: The readTreeOids method currently splits git ls-tree
output on newlines and tabs which fails for paths containing those characters;
update the plumbing.execute call inside readTreeOids to run git ls-tree with the
-z flag (args: ['ls-tree', '-r', '-z', treeOid]) and then parse the
NUL-separated records by splitting on '\0', skipping empty entries, and
extracting meta and path reliably from each record (use the last NUL-separated
segment as path and parse the preceding meta to get the oid); ensure
_validateOid(treeOid) remains and return the same oids object mapping path ->
oid.

🧹 Nitpick comments (1)

src/infrastructure/adapters/GitGraphAdapter.js (1)

140-175: Don’t swallow unexpected rev-parse failures.

Catching all errors and returning null can hide repo corruption or plumbing issues. Consider only returning null for “missing ref” errors and rethrow otherwise.

✅ Proposed refinement

  async readRef(ref) {
    this._validateRef(ref);
    try {
      const oid = await this.plumbing.execute({
        args: ['rev-parse', ref]
      });
      return oid.trim();
-   } catch {
-     return null;
+   } catch (err) {
+     const msg = err?.stderr || err?.message || '';
+     if (msg.includes('unknown revision') || msg.includes('bad revision')) {
+       return null;
+     }
+     throw err;
    }
  }

Please confirm the actual error shape from plumbing.execute to tune the check.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 163-172: The readRef method currently swallows all errors and
returns null; update readRef (and keep this._validateRef and the
this.plumbing.execute call) to only return null when the underlying error
indicates the ref truly doesn't exist (match Git "not found" patterns like
"unknown revision", "fatal: ambiguous argument", "fatal: no such ref", or
"fatal: ambiguous argument" in the error.message/output), and rethrow any other
errors (permissions, I/O, corruption) so they propagate normally.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 20-28: The commitNode, writeBlob, and writeTree methods currently
return raw stdout from plumbing.execute which includes a trailing newline;
update each of these methods to trim the plumbing.execute() result (e.g., call
.trim() on the stdout string) before returning to ensure OIDs are normalized;
you can follow readRef()'s approach—locate commitNode, writeBlob, and writeTree
and return the trimmed string from the plumbing.execute call so downstream
validation (e.g., _validateOid, CacheRebuildService tree entries) won't fail.