From bb4c0bfb9951ce6f51a5283809b6dc8a843e9879 Mon Sep 17 00:00:00 2001 From: David Snelling Date: Tue, 4 Nov 2025 15:39:58 -0800 Subject: [PATCH] fix: add NULL hash guards to prevent v5.3.3 regression Fixed critical bug where CommitObject.walk() didn't guard against NULL parent hash, causing "Blob metadata not found: 0000...0000" error when calling getHistory() on fresh databases. This is a SYSTEMATIC fix, not just a patch: Infrastructure: - src/storage/cow/constants.ts: NEW - NULL_HASH constant and utilities - Prevents hardcoding errors - Provides isNullHash() for semantic checks Bug Fixes: - src/storage/cow/CommitObject.ts: Guard NULL parent in walk() - src/storage/cow/BlobStorage.ts: Defensive check rejects NULL hash reads - src/storage/baseStorage.ts: Use NULL_HASH constant (not hardcoded) - src/brainy.ts: Use NULL_HASH constant (not hardcoded) Testing: - tests/integration/initial-commit-null-parent.test.ts: 5 regression tests - All 17 tests pass (5 new + 12 existing COW tests) - Zero regressions This fix addresses the root cause of 4 consecutive bugs (v5.3.0-v5.3.3): missing defensive programming for sentinel values in COW storage. Fixes: Workshop team bug report (v5.3.3 regression) Tests: 17/17 pass (5 new regression + 12 existing COW) Build: SUCCESSFUL (zero errors) --- src/brainy.ts | 5 +- src/storage/baseStorage.ts | 4 +- src/storage/cow/BlobStorage.ts | 13 ++ src/storage/cow/CommitObject.ts | 8 +- src/storage/cow/constants.ts | 59 ++++++++ .../initial-commit-null-parent.test.ts | 143 ++++++++++++++++++ 6 files changed, 228 insertions(+), 4 deletions(-) create mode 100644 src/storage/cow/constants.ts create mode 100644 tests/integration/initial-commit-null-parent.test.ts diff --git a/src/brainy.ts b/src/brainy.ts index cb5028e1..10b30d10 100644 --- a/src/brainy.ts +++ b/src/brainy.ts @@ -2357,9 +2357,12 @@ export class Brainy implements BrainyInterface { const entityCount = await this.getNounCount() const relationshipCount = await this.getVerbCount() + // v5.3.4: Import NULL_HASH constant + const { NULL_HASH } = await import('./storage/cow/constants.js') + // Build commit object using builder pattern const builder = CommitBuilder.create(blobStorage) - .tree('0000000000000000000000000000000000000000000000000000000000000000') // Empty tree hash for now + .tree(NULL_HASH) // Empty tree hash (sentinel value) .message(options?.message || 'Snapshot commit') .author(options?.author || 'unknown') .timestamp(Date.now()) diff --git a/src/storage/baseStorage.ts b/src/storage/baseStorage.ts index 3c9a823c..12da72f0 100644 --- a/src/storage/baseStorage.ts +++ b/src/storage/baseStorage.ts @@ -303,7 +303,9 @@ export abstract class BaseStorage extends BaseStorageAdapter { const mainRef = await this.refManager.getRef('main') if (!mainRef) { // Create initial commit with empty tree - const emptyTreeHash = '0000000000000000000000000000000000000000000000000000000000000000' + // v5.3.4: Use NULL_HASH constant instead of hardcoded string + const { NULL_HASH } = await import('./cow/constants.js') + const emptyTreeHash = NULL_HASH // Import CommitBuilder const { CommitBuilder } = await import('./cow/CommitObject.js') diff --git a/src/storage/cow/BlobStorage.ts b/src/storage/cow/BlobStorage.ts index 79a8212d..c8a33ae8 100644 --- a/src/storage/cow/BlobStorage.ts +++ b/src/storage/cow/BlobStorage.ts @@ -15,6 +15,7 @@ */ import { createHash } from 'crypto' +import { NULL_HASH, isNullHash } from './constants.js' /** * Simple key-value storage interface for COW primitives @@ -257,6 +258,18 @@ export class BlobStorage { * @returns Blob data */ async read(hash: string, options: BlobReadOptions = {}): Promise { + // v5.3.4 fix: Guard against NULL hash (sentinel value) + // NULL_HASH ('0000...0000') is used as a sentinel for "no parent" or "empty tree" + // It should NEVER be read as actual blob data + if (isNullHash(hash)) { + throw new Error( + `Cannot read NULL hash (${NULL_HASH}): ` + + `This is a sentinel value indicating "no parent commit" or "empty tree". ` + + `If you're seeing this error from CommitObject.walk(), there's a bug in commit traversal logic. ` + + `If you're seeing this from TreeObject operations, there's a bug in tree handling.` + ) + } + // Check cache first if (!options.skipCache) { const cached = this.getFromCache(hash) diff --git a/src/storage/cow/CommitObject.ts b/src/storage/cow/CommitObject.ts index 193e8f7d..f22c2fa5 100644 --- a/src/storage/cow/CommitObject.ts +++ b/src/storage/cow/CommitObject.ts @@ -16,6 +16,7 @@ */ import { BlobStorage } from './BlobStorage.js' +import { isNullHash } from './constants.js' /** * Commit object structure @@ -336,7 +337,10 @@ export class CommitObject { let currentHash: string | null = startHash let depth = 0 - while (currentHash) { + // v5.3.4 fix: Guard against NULL hash (sentinel for "no parent") + // The initial commit has parent = null or NULL_HASH ('0000...0000') + // We must stop walking when we reach it, not try to read it + while (currentHash && !isNullHash(currentHash)) { // Check max depth if (options?.maxDepth && depth >= options.maxDepth) { break @@ -364,7 +368,7 @@ export class CommitObject { break } - // Move to parent + // Move to parent (can be null or NULL_HASH for initial commit) currentHash = commit.parent depth++ } diff --git a/src/storage/cow/constants.ts b/src/storage/cow/constants.ts new file mode 100644 index 00000000..0dd05a42 --- /dev/null +++ b/src/storage/cow/constants.ts @@ -0,0 +1,59 @@ +/** + * COW Storage Constants + * + * Sentinel values and utilities for Copy-On-Write storage system. + * + * @module storage/cow/constants + */ + +/** + * NULL_HASH - Sentinel value for "no parent commit" or "empty tree" + * + * In Git-like COW systems, we need a way to represent: + * - Initial commit (has no parent) + * - Empty tree (contains no files) + * + * We use a 64-character zero hash as a sentinel value. + * This should NEVER be used as an actual content hash. + * + * @constant + * @example + * ```typescript + * const builder = CommitBuilder.create(storage) + * .tree(NULL_HASH) // Empty tree + * .parent(null) // No parent (use null, not NULL_HASH) + * .build() + * ``` + */ +export const NULL_HASH = '0000000000000000000000000000000000000000000000000000000000000000' + +/** + * Check if a hash is the NULL sentinel value + * + * @param hash - Hash to check (can be string or null) + * @returns true if hash is null or NULL_HASH + * + * @example + * ```typescript + * if (isNullHash(commit.parent)) { + * console.log('This is the initial commit') + * } + * ``` + */ +export function isNullHash(hash: string | null | undefined): boolean { + return hash === null || hash === undefined || hash === NULL_HASH +} + +/** + * Check if a hash is valid (non-null, non-empty, proper format) + * + * @param hash - Hash to check + * @returns true if hash is a valid SHA-256 hash + */ +export function isValidHash(hash: string | null | undefined): boolean { + if (isNullHash(hash)) { + return false + } + // SHA-256 hash must be exactly 64 hexadecimal characters + return typeof hash === 'string' && /^[a-f0-9]{64}$/.test(hash) +} diff --git a/tests/integration/initial-commit-null-parent.test.ts b/tests/integration/initial-commit-null-parent.test.ts new file mode 100644 index 00000000..7a97c7f4 --- /dev/null +++ b/tests/integration/initial-commit-null-parent.test.ts @@ -0,0 +1,143 @@ +/** + * Regression test for v5.3.3 NULL parent bug + * + * BUG: CommitObject.walk() didn't guard against NULL parent hash, causing + * "Blob metadata not found: 0000...0000" error when calling getHistory() + * on a fresh database with only the initial commit. + * + * ROOT CAUSE: Initial commit has parent = null or NULL_HASH ('0000...0000'), + * but while(currentHash) treated the string as truthy and tried to read it. + * + * FIXED IN: v5.3.4 + * - Added isNullHash() guard in CommitObject.walk() + * - Added defensive check in BlobStorage.read() + * - Created NULL_HASH constant to prevent hardcoding + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { Brainy } from '../../src/brainy.js' +import { NULL_HASH } from '../../src/storage/cow/constants.js' +import * as fs from 'fs' + +const TEST_DATA_PATH = './test-null-parent-data' + +describe('Initial Commit NULL Parent Bug (v5.3.3 regression)', () => { + let brain: Brainy + + beforeEach(async () => { + // Clean up test data + if (fs.existsSync(TEST_DATA_PATH)) { + fs.rmSync(TEST_DATA_PATH, { recursive: true, force: true }) + } + + brain = new Brainy({ + storage: { + type: 'filesystem', + path: TEST_DATA_PATH, + branch: 'main', + enableCompression: true + }, + disableAutoRebuild: true, + silent: true + }) + + await brain.init() + }) + + afterEach(() => { + if (fs.existsSync(TEST_DATA_PATH)) { + fs.rmSync(TEST_DATA_PATH, { recursive: true, force: true }) + } + }) + + it('should handle getHistory() on fresh database with only initial commit', async () => { + // Fresh brain has only the initial commit (created during init) + // This was throwing "Blob metadata not found: 0000...0000" in v5.3.3 + + // Get history - should NOT throw + const history = await brain.getHistory({ limit: 10 }) + + // Should return the initial commit + expect(history).toBeDefined() + expect(Array.isArray(history)).toBe(true) + expect(history.length).toBe(1) + + // Initial commit should have specific properties + const initialCommit = history[0] + expect(initialCommit.message).toBe('Initial commit') + expect(initialCommit.author).toBe('system') + // Parent can be null or undefined for initial commit + expect(initialCommit.parent == null).toBe(true) + }) + + it('should stop walk at initial commit (parent = null)', async () => { + // Note: brain.commit() creates "Snapshot commit" messages by default + // Create 2 commits + await brain.add({ data: 'First entity', type: 'concept' }) + await brain.commit('First user commit') + + await brain.add({ data: 'Second entity', type: 'concept' }) + await brain.commit('Second user commit') + + // Get full history + const history = await brain.getHistory({ limit: 100 }) + + // Should have at least 3 commits (2 user + 1 initial) + expect(history.length).toBeGreaterThanOrEqual(3) + + // Last commit should be initial commit with no parent + const initialCommit = history[history.length - 1] + expect(initialCommit.message).toBe('Initial commit') + // Parent can be null or undefined for initial commit + expect(initialCommit.parent == null).toBe(true) + }) + + it('should not attempt to read NULL hash from BlobStorage', async () => { + // Direct test: attempting to read NULL_HASH should throw clear error + const blobStorage = (brain as any).storage.blobStorage + + // Should throw clear error message + await expect( + blobStorage.read(NULL_HASH) + ).rejects.toThrow(/sentinel value/) + + // Error message should mention what NULL_HASH is for + await expect( + blobStorage.read(NULL_HASH) + ).rejects.toThrow(/no parent/) + }) + + it('should handle multiple calls to getHistory() consistently', async () => { + // Ensure caching doesn't break NULL hash handling + + // First call + const history1 = await brain.getHistory({ limit: 10 }) + expect(history1.length).toBe(1) + + // Add a commit + await brain.add({ data: 'Test entity', type: 'concept' }) + await brain.commit('Test commit') + + // Second call (should now have 2 commits) + const history2 = await brain.getHistory({ limit: 10 }) + expect(history2.length).toBe(2) + + // Third call (should still have 2 commits) + const history3 = await brain.getHistory({ limit: 10 }) + expect(history3.length).toBe(2) + + // All should end with initial commit + expect(history1[history1.length - 1].message).toBe('Initial commit') + expect(history2[history2.length - 1].message).toBe('Initial commit') + expect(history3[history3.length - 1].message).toBe('Initial commit') + }) + + it('should handle walking with maxDepth on initial commit', async () => { + // Edge case: maxDepth = 1 on fresh database + + const history = await brain.getHistory({ limit: 1 }) + + expect(history.length).toBe(1) + expect(history[0].message).toBe('Initial commit') + }) +})