Skip to content

Upgrade Harper to v5#2

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

Upgrade Harper to v5#2
BboyAkers wants to merge 3 commits into
mainfrom
v5-upgrade

Conversation

@BboyAkers

Copy link
Copy Markdown
Member

Summary

  • Dependency: Added harper@^5.0.28 as an explicit dependency (was previously a runtime-provided global; no harperdb was ever listed in package.json)
  • Integration tests: Added integrationTests/twilio-sms.test.ts covering the POST /optInStatus endpoint (opt-in/opt-out keyword handling) and the PhoneNumbers REST table (CRUD operations)
  • CI workflow: Added .github/workflows/integration-tests.yml with Node.js 22/24/26 matrix on ubuntu-latest
  • harperBinPath fix: Applied createRequire-based bin path resolution to bypass ERR_PACKAGE_PATH_NOT_EXPORTED (harper's exports map only exposes ".")
  • 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
  • Branding: HarperDBHarper in README prose; npm i -g harperdbnpm i -g harper; harperdb devharper dev
  • TypeScript: Added tsconfig.json

Migration items

Item Status
harperdbharper package rename N/A — no explicit dep existed; added harper@^5
Table.get() returns plain/frozen records N/A — resources.js uses .put() only, no .get()
blob.save()saveBeforeCommit N/A — no blob usage
wasLoadedFromSource().loadedFromSource N/A — no caching logic
Child process spawn allowlist N/A — no child_process usage
Install scripts N/A — no install scripts
Instance vs static handlers Already correct — post is an instance method on optInStatus class
harperdb devharper dev Applied in dev script

Test scope and known limitations

Twilio credentials are not available in CI. Tests validate the Harper integration layer only:

  • The POST /optInStatus endpoint routing and data persistence (OptOutType keyword → PhoneNumbers table write)
  • The GET /PhoneNumbers REST table interface
  • No actual Twilio SMS sending is tested (end-to-end Twilio connectivity requires a live Twilio account with a provisioned number)

Local test note: npm run test:integration cannot run on macOS without loopback alias configuration (sudo npx harper-integration-test-setup-loopback). CI (ubuntu-latest) supports the full 127.0.0.0/8 range natively and is the test gate.

npm scope

The package is currently unscoped (twilio-sms). If it should be published under the Harper npm scope, that migration is manual and flagged for human decision (per upgrade plan §11.1).

🤖 Generated with Claude Code

…inPath fix

- Add harper@^5.0.28 as explicit dependency (was runtime-provided global)
- Add @harperfast/integration-testing and typescript dev dependencies
- Regenerate package-lock.json with --os=linux --cpu=x64 --include=optional
  to include bufferutil/utf-8-validate/node-gyp-build for Linux CI
- Add integrationTests/twilio-sms.test.ts covering optInStatus POST endpoint
  and PhoneNumbers REST table (Harper layer only; Twilio credentials not
  available in CI — SMS sending not tested)
- Apply harperBinPath fix (ERR_PACKAGE_PATH_NOT_EXPORTED workaround)
- Add .github/workflows/integration-tests.yml with Node 22/24/26 matrix
- Update dev script: harperdb dev → harper dev
- Branding: "HarperDB" → "Harper" in README prose; update install command
  to npm i -g harper; fix description in package.json
- Add tsconfig.json

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

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Comment thread integrationTests/twilio-sms.test.ts Outdated
ok([200, 201, 204].includes(res.status), `expected 2xx for opt-out, got HTTP ${res.status}`);

// Verify the record was written to the PhoneNumbers table
const record = await authFetch(ctx, `/PhoneNumbers/${encodeURIComponent(from)}`);

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: Verification GET uses encodeURIComponent but the record key was written as a raw phone number

tables.PhoneNumbers.put({ phoneNumber: From, ... }) stores the key as the literal From value (e.g. +15551110001). The verification fetch here uses encodeURIComponent(from)/PhoneNumbers/%2B15551110001, which is a different URL path than /PhoneNumbers/+15551110001. Other Harper repos (puppeteer-component, realtime-pubsub-example) consistently use encodeURIComponent for all REST path operations. The same asymmetry affects lines 135, 150, and 165.

Either:

  • Remove encodeURIComponent here (if Harper normalises %2B+ in path lookups), or
  • Use encodeURIComponent in the earlier direct PUT tests too (lines 68, 78, 84) — this is the recommended pattern.
Suggested change
const record = await authFetch(ctx, `/PhoneNumbers/${encodeURIComponent(from)}`);
const record = await authFetch(ctx, `/PhoneNumbers/${from}`);

@@ -0,0 +1,180 @@
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 is imported on this line but does not appear anywhere else in the file. Remove it to keep the import clean.

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

@BboyAkers

Copy link
Copy Markdown
Member Author

Code review finding on unchanged file resources.js:

updateStatus (line 6) has no null guard on OptOutType

OptOutType.toLowerCase() is called unconditionally inside updateStatus. The only guard is the if (data.OptOutType) check in post(). If updateStatus is ever called directly (unit test, future refactor, or unexpected caller) with a null/undefined OptOutType, it will throw TypeError: Cannot read properties of undefined (reading "toLowerCase").

Suggested fix:

function updateStatus(data) {
  const { OptOutType, From } = data;
  if (!OptOutType || !From) return;
  const optOut = OptOutType.toLowerCase();
  // ...
}

This finding is on resources.js which is not part of this PR diff, but since the new integration tests exercise updateStatus and the PR touches the surrounding infrastructure, it is worth noting.

Comment thread integrationTests/twilio-sms.test.ts Outdated

void test('Harper starts successfully and root is reachable', async () => {
const res = await authFetch(ctx, '/');
ok([200, 400, 404].includes(res.status), `Unexpected status ${res.status}`);

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.

Smoke test accepts HTTP 400 — a request error masquerading as success

A 400 response means Harper rejected the request (bad input, config error, routing issue). Accepting it as a passing smoke test means startup failures that cause malformed responses would go undetected. The smoke test should only accept 200 (or at most 404 for a missing root route).

Comment thread tsconfig.json
// See also https://aka.ms/tsconfig/module
"module": "nodenext",
"target": "esnext",
"types": [],

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: tsconfig.json sets "types": [] but the test file uses Buffer (line 25)

With "types": [], ambient Node.js type declarations are excluded, so tsc --noEmit would fail on the Buffer.from(...) call in authFetch. At runtime this is fine (Node native TS stripping ignores tsconfig), but the tsconfig is misleading and would block any future type-checking step.

Either add @types/node to devDependencies and include "node" in "types", or add a // @ts-ignore / use btoa() instead of Buffer.from(...).toString("base64").

…false-pass, and updateStatus null guard

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