Skip to content

Upgrade Harper to v5 and add integration tests#8

Open
BboyAkers wants to merge 3 commits into
mainfrom
v5-upgrade
Open

Upgrade Harper to v5 and add integration tests#8
BboyAkers wants to merge 3 commits into
mainfrom
v5-upgrade

Conversation

@BboyAkers

Copy link
Copy Markdown
Member

Summary

  • Dependency bump: added harper@^5.0.28 as an explicit dependency (was runtime-provided global). harperdb was never listed in package.json — this repo relied on the global being injected.
  • Migration fix: replaced non-standard table.update({ where, data }) call in the ModelFetchJobs retry handler with a v5-compatible table.put({ ...job, ...updates }) (frozen-record-safe spread).
  • Integration tests: added integrationTests/data-layer.test.ts using @harperfast/integration-testing@0.4.0. Tests cover the Harper data layer (Model, InferenceEvent, BenchmarkResult, Feature, ModelFetchJob table CRUD + REST resource endpoints + Status health check). AI inference endpoints (Predict/Personalize/Benchmark) require external model files, GPU, or running Ollama — these are exercised only for their data-path error handling in CI.
  • CI: added .github/workflows/integration-tests.yml with Node 22/24/26 matrix on ubuntu-latest. Sets MODEL_FETCH_WORKER=false to disable the background model-fetch worker during tests (avoids download attempts).
  • Lockfile: regenerated package-lock.json with --os=linux --cpu=x64 --include=optional to include bufferutil, utf-8-validate, and node-gyp-build for Linux CI.
  • TypeScript: added tsconfig.json targeting NodeNext with erasableSyntaxOnly for the integration test suite.
  • Branding: HarperDBHarper in README.md, CONTRIBUTING.md, ROADMAP.md, package.json description/author/keywords. Clone URL updated to HarperFast/edge-ai-ops.
  • Engines: bumped engines.node to >=22.0.0 (required by @harperfast/integration-testing).

Migration items applied

Item Status
harperdbharper import/dep N/A — runtime-provided global; no explicit import existed
wasLoadedFromSource() N/A — not used
blob.save() N/A — not used
Frozen records (no direct property mutation) Applied — table.update({where,data}) replaced with put({ ...job, ...updates })
Transaction context (getContext()) N/A — no explicit transaction usage
Table.get() return shape N/A — .wasLoadedFromSource() not called
Process spawning (allowedSpawnCommands) N/A — no child_process spawn calls

AI inference note

TensorFlow.js, ONNX Runtime, Transformers.js, and Ollama backends require external model files (hundreds of MB) or a running Ollama service. These are not available in CI. The integration tests cover the Harper data layer only, exercising:

  • Table CRUD (Model, InferenceEvent, Feature, BenchmarkResult, ModelFetchJob)
  • Custom resource endpoints (Status, Predict error path, WorkerControl, ModelList)

Known issues / flagged items

  • npm scope: package.json is named harper-edge-ai-example (unscoped). If this should move to the @harperfast/ npm scope, that migration is manual — flagged for human review per org process §11.1.
  • Local integration tests: will fail with EADDRNOTAVAIL on macOS (loopback aliases not configured). CI on ubuntu-latest is the test gate.
  • Lint warnings: pre-existing prettier/eslint warnings in scripts/ and examples/ are not introduced by this PR. ESLint is not blocking (errors are in scripts, not the Harper app code).

Test results

  • Local: blocked by EADDRNOTAVAIL (macOS loopback — environmental, not a code bug)
  • CI: running on push — see GitHub Actions for Node 22/24/26 results

🤖 Generated with Claude Code

- Add harper@5.0.28 as an explicit dependency (was runtime-provided global)
- Add @harperfast/integration-testing@0.4.0 + typescript devDeps
- Replace non-standard table.update({where,data}) with v5 put+spread in ModelFetchJobs retry handler
- Add integrationTests/data-layer.test.ts covering Harper table CRUD and REST resource endpoints (data layer only; AI inference skipped — requires external model files not available in CI)
- Add .github/workflows/integration-tests.yml with Node 22/24/26 matrix on ubuntu-latest
- Add tsconfig.json for TypeScript integration test compilation
- Regenerate package-lock.json with --os=linux --cpu=x64 --include=optional to include bufferutil/utf-8-validate/node-gyp-build for CI
- Update test:integration script to use harper-integration-test-run
- Bump engines.node to >=22.0.0 to match @harperfast/integration-testing requirement
- Branding: HarperDB → Harper in README, CONTRIBUTING.md, ROADMAP.md, package.json
- Fix repository URL and GitHub clone URL to HarperFast/edge-ai-ops

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the project for Harper v5, renaming references from HarperDB to Harper, bumping the minimum Node.js version to 22, adding a new integration test suite, and refactoring the model fetch job reset logic to avoid frozen-record mutations. Key feedback includes correcting a non-existent TypeScript version in package.json, utilizing native import.meta.resolve in tests, adding defensive checks for the test context, and strengthening test assertions for the /ModelList endpoint.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread package.json
"lint-staged": "^16.2.7",
"prettier": "^3.6.2"
"prettier": "^3.6.2",
"typescript": "^6.0.3"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

TypeScript version ^6.0.3 does not exist on npm (the current major version is 5.x). Specifying a non-existent version will cause npm install to fail with an ETARGET error. Please downgrade this to a valid stable version, such as ^5.7.3.

Suggested change
"typescript": "^6.0.3"
"typescript": "^5.7.3"

Comment on lines +22 to +32
import { fileURLToPath } from 'node:url';
import { dirname, resolve } from 'node:path';
import { createRequire } from 'node:module';

const __dirname = dirname(fileURLToPath(import.meta.url));
const FIXTURE_PATH = resolve(__dirname, '..');

// harper's `exports` only exposes ".", so 'harper/dist/bin/harper.js' is not resolvable.
// Resolve the CLI from the exported main entry and pass it explicitly.
const require = createRequire(import.meta.url);
const harperBinPath = resolve(dirname(require.resolve('harper')), 'bin/harper.js');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since the project requires Node.js >=22.0.0, you can leverage the stable, native import.meta.resolve instead of creating a legacy CommonJS require context via createRequire to resolve the harper package path.

Suggested change
import { fileURLToPath } from 'node:url';
import { dirname, resolve } from 'node:path';
import { createRequire } from 'node:module';
const __dirname = dirname(fileURLToPath(import.meta.url));
const FIXTURE_PATH = resolve(__dirname, '..');
// harper's `exports` only exposes ".", so 'harper/dist/bin/harper.js' is not resolvable.
// Resolve the CLI from the exported main entry and pass it explicitly.
const require = createRequire(import.meta.url);
const harperBinPath = resolve(dirname(require.resolve('harper')), 'bin/harper.js');
import { fileURLToPath } from 'node:url';
import { dirname, resolve } from 'node:path';
const __dirname = dirname(fileURLToPath(import.meta.url));
const FIXTURE_PATH = resolve(__dirname, '..');
// harper's `exports` only exposes ".", so 'harper/dist/bin/harper.js' is not resolvable.
// Resolve the CLI from the exported main entry and pass it explicitly.
const harperMainPath = fileURLToPath(import.meta.resolve('harper'));
const harperBinPath = resolve(dirname(harperMainPath), 'bin/harper.js');

Comment on lines +34 to +47
function authFetch(
ctx: ContextWithHarper,
path: string,
init: RequestInit & { headers?: Record<string, string> } = {},
) {
const { headers = {}, ...rest } = init;
const creds = Buffer.from(
`${ctx.harper.admin.username}:${ctx.harper.admin.password}`,
).toString('base64');
return fetch(`${ctx.harper.httpURL}${path}`, {
...rest,
headers: { Authorization: `Basic ${creds}`, ...headers },
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent cryptic TypeError: Cannot read properties of undefined errors if the Harper test environment setup fails or is skipped, add a defensive check to ensure ctx.harper and ctx.harper.admin are fully initialized before attempting to access their properties.

function authFetch(
  ctx: ContextWithHarper,
  path: string,
  init: RequestInit & { headers?: Record<string, string> } = {},
) {
  if (!ctx?.harper?.admin) {
    throw new Error('Harper test instance is not initialized. Ensure setupHarperWithFixture completed successfully.');
  }
  const { headers = {}, ...rest } = init;
  const creds = Buffer.from(
    `${ctx.harper.admin.username}:${ctx.harper.admin.password}`,
  ).toString('base64');
  return fetch(`${ctx.harper.httpURL}${path}`, {
    ...rest,
    headers: { Authorization: `Basic ${creds}`, ...headers },
  });
}

Comment on lines +122 to +126
void test('GET /ModelList returns array (no auth required if MODEL_FETCH_AUTH not set)', async () => {
const res = await authFetch(ctx, '/ModelList');
// May return 200 (list) or 401 (auth required) depending on env; both are valid behaviours.
ok([200, 401, 403].includes(res.status), `unexpected status ${res.status}`);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When GET /ModelList returns a 200 OK status, the test should also verify that the response body is indeed an array, ensuring the contract of the endpoint is met.

  void test('GET /ModelList returns array (no auth required if MODEL_FETCH_AUTH not set)', async () => {
    const res = await authFetch(ctx, '/ModelList');
    // May return 200 (list) or 401/403 (auth required) depending on env; both are valid behaviours.
    ok([200, 401, 403].includes(res.status), `unexpected status ${res.status}`);
    if (res.status === 200) {
      const body = await res.json();
      ok(Array.isArray(body), 'expected array');
    }
  });

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Comment thread src/resources.js Outdated
errorCode: null
}
// Reset job to queued (v5: use put with spread to avoid frozen-record mutation)
await tables.ModelFetchJob.put({

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: ...job spread skips date-string coercion that ModelFetchWorker.updateJobStatus performs

When Harper v5 serializes a record and returns it via tables.ModelFetchJob.get(), date fields (createdAt, startedAt, completedAt) may come back as ISO-8601 strings instead of integer timestamps. ModelFetchWorker.updateJobStatus (line 473–482) explicitly coerces these with new Date(...).getTime() before calling put(). This put call copies them verbatim via ...job, so if Harper returns a string date the put will store a string in a Date field, causing downstream sort/comparison failures (e.g. the createdAt sort in GET).

Fix: apply the same coercion logic before put, or extract it into a shared helper:

// Reset job to queued (v5: use put with spread to avoid frozen-record mutation)
const jobData = {
  ...job,
  status: 'queued',
  retryCount: 0,
  lastError: null,
  errorCode: null,
};
if (typeof jobData.createdAt === 'string') jobData.createdAt = new Date(jobData.createdAt).getTime();
if (typeof jobData.startedAt === 'string') jobData.startedAt = new Date(jobData.startedAt).getTime();
if (typeof jobData.completedAt === 'string') jobData.completedAt = new Date(jobData.completedAt).getTime();
await tables.ModelFetchJob.put(jobData);

await setupHarperWithFixture(ctx, FIXTURE_PATH, { harperBinPath });
});

after(async () => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: teardownHarper(ctx) will throw if setupHarperWithFixture failed

If setupHarperWithFixture throws (e.g. Harper fails to start), ctx.harper is never populated. The after() hook still calls teardownHarper(ctx), which is typed as requiring StartedHarperTestContext (i.e. ctx.harper must be defined). This crashes teardown with a confusing secondary error that masks the real startup failure.

Fix: guard teardown against the missing property:

after(async () => {
  if ((ctx as any).harper) {
    await teardownHarper(ctx);
  }
});

*/

import { suite, test, before, after } from 'node:test';
import { strictEqual, ok, deepStrictEqual } from 'node:assert/strict';

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup: deepStrictEqual is imported but never used

deepStrictEqual appears in the import on this line but is not called anywhere in the file. Remove it to avoid lint warnings and keep the import surface clean.

import { strictEqual, ok } from 'node:assert/strict';

Comment thread package.json
"test": "node --test tests/unit/**/*.test.js",
"test:unit": "node --test tests/unit/**/*.test.js",
"test:integration": "node --test tests/integration/**/*.test.js",
"test:integration": "harper-integration-test-run integrationTests/**/*.test.ts",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: test:all silently drops all 9 files in tests/integration/

Before this PR, test:integration ran node --test tests/integration/**/*.test.js (9 files, including model-fetch-system.test.js, predict.test.js, etc.). After this change it runs harper-integration-test-run integrationTests/**/*.test.ts — an entirely different directory. The old tests/integration/ files are now only reachable via test:benchmark (which only covers benchmark-comparison.test.js). The remaining 8 integration test files are silently orphaned and no longer run in CI.

Either the old tests should be removed (if they're superseded by the new Harper-native suite), or test:all needs to also run them, or they need a new npm script entry.

NODE_VER: ${{ github.event.inputs.node-version }}
run: |
if [ "$NODE_VER" == "all" ] || [ -z "$NODE_VER" ]; then
echo "node-versions=[22, 24, 26]" >> $GITHUB_OUTPUT

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: empty NODE_VER on pull_request triggers falls through to the else branch and produces an empty matrix

On push and pull_request events, github.event.inputs.node-version is null/empty, so NODE_VER is empty. The condition [ -z "$NODE_VER" ] correctly catches this on line 36 — but this relies on the || -z arm being evaluated before the else. That part is fine. However, if the all case is accidentally removed or the condition inverted in future, an empty NODE_VER would produce node-versions=[], causing fromJSON() to yield an empty array and the matrix to silently run zero jobs — CI would show green with no tests executed.

Consider making the default explicit with a comment, or asserting non-empty in the else branch, to make the silent-skip risk visible.

- Coerce ISO-8601 date strings to integer timestamps before retry put in
  ModelFetchJobs.post, matching the pattern in ModelFetchWorker.updateJobStatus,
  to prevent FIFO sort failures and schema violations on Harper v5
- Guard teardownHarper in data-layer.test.ts after() so a startup failure
  surfaces the real HarperStartupError instead of a secondary TypeError
- Add test:integration:legacy script for tests/integration/**/*.test.js and
  include it in test:all to prevent those 9 files from being silently orphaned

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@BboyAkers

Copy link
Copy Markdown
Member Author

Review follow-up (autonomous agent): Fixed all 3 blocking findings: coerce ISO-8601 date strings to integer timestamps before retry put in ModelFetchJobs.post (matching ModelFetchWorker.updateJobStatus pattern); guard teardownHarper in after() to surface real startup errors; add test:integration:legacy script to include orphaned tests/integration/ files in CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant