Add test to confirm we can downgrade to older harperdb

[kriszyp]

Dependent on https://github.com/HarperFast/harperdb/pull/3114

[claude]

Review: PR #406 — Add test to confirm we can downgrade to older harperdb

Surfaces traced: integrationTests/upgrade/4.x-upgrade.test.ts, integrationTests/utils/harperLifecycle.ts (for context on startHarper, killHarper, legacyPath narrowing).

---

1. existsSync result discarded — RocksDB assertion is a no-op

File: integrationTests/upgrade/4.x-upgrade.test.ts:117

What: existsSync(join(ctx.harper.dataRootDir, 'database', 'system', 'CURRENT')) is called but its return value is never passed to ok() or any assertion. The test unconditionally passes regardless of whether the RocksDB CURRENT marker file actually exists.

Why it matters: The whole point of that line is to confirm the LMDB → RocksDB migration completed. Without the assertion, the 'upgrade and migrate LMDB to RocksDB' test gives false confidence — a broken migration produces a green test.

Suggested fix: Inline comment posted with a code suggestion wrapping the call in ok(...).

---

No other blockers. The downgrade test logic itself is correct: kills v5, restarts on the legacy binary with CONFIRM_DOWNGRADE=yes, and verifies data integrity via both search_by_conditions and read_audit_log. The legacyPath variable move to suite scope is necessary to share it between the two tests that need it, and the early-return guard in before() covers the "env var not set" case for setup.

One minor nit (not a blocker): the comment on line 84 reads // start on v5 but the code is starting the legacy binary — should be something like // start on legacy v4.x.

[claude]

Bug: TypeError when HARPER_LEGACY_VERSION_PATH is unset

legacyPath was promoted to suite scope so the new test body can reference it — but unlike before(), the test body has no guard. When the env var is absent legacyPath is undefined, and path.join(undefined, 'bin', 'harperdb.js') throws a TypeError rather than gracefully skipping.

Suggested change

	harperBinPath: join(legacyPath, 'bin', 'harperdb.js'),
	harperBinPath: join(legacyPath!, 'bin', 'harperdb.js'),

Or, add an early-return guard consistent with before():

test('downgrade and start', async () => {
    if (!legacyPath) return;
    await killHarper(ctx);
    ...

The ! non-null assertion is enough if this test file is always run in an environment where the env var is set, but the explicit return guard is safer and matches the pattern already used in before().

[claude]

Review: PR #406 — Add downgrade test + system DB migration fix

1. TypeError in downgrade and start when env var is unset

File: integrationTests/upgrade/4.x-upgrade.test.ts:83
What: legacyPath was moved from a before()-local const to suite scope so the new test body can reference it. before() guards with if (!legacyPath) return; but the new downgrade and start test body does not. If HARPER_LEGACY_VERSION_PATH is absent, path.join(undefined, 'bin', 'harperdb.js') throws a TypeError at test-run time rather than silently skipping.
Why it matters: Every other test in the suite either doesn't touch legacyPath in its body or fails in less-explosive ways; this test alone throws an unhandled TypeError.
Suggested fix: Add if (!legacyPath) return; as the first line of the test body, matching the guard already in before().

---

Surfaces verified (no other blockers)

• openRocksDb API change (bin/copyDb.ts:292): RocksDatabase.open(new RocksIndexStore(...)) → new RocksIndexStore(...).open(). Correct given the upstream API change in harperdb#3114; DRAFT status appropriately gates this on that PR landing.
• System DB inclusion in migrateOnStart (bin/copyDb.ts:310-313): The original for...in + if (databaseName === 'system') continue was dead code because system is a non-enumerable property and for...in never enumerates it. The fix (Object.keys + explicit push) is correct and the new existsSync assertion on database/system/CURRENT directly verifies the fix works.
• RocksDB presence checks (4.x-upgrade.test.ts:117-118): Both data and system CURRENT-file checks are valid markers for a completed RocksDB migration.
• No new runtime dependencies introduced; dependencies.md does not need updating.
• TypeStrip compatibility: No non-erasable TypeScript constructs in the diff.

[claude]

Reviewed; no blockers found.

[cb1kenobi]

Good catch, but I now realize that RocksIndexStore isn't a "store" anymore and I approved the PR!