Skip to content

Add integration-tests CI and fix harness CLI resolution #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BboyAkers wants to merge 3 commits into
base: main
Choose a base branch
from
+81 −1
Open

Add integration-tests CI and fix harness CLI resolution #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1217f8
Add integration-tests CI and fix harness CLI resolution
BboyAkers Jun 5, 2026
e02789d
Update integrationTests/app.test.ts
BboyAkers Jun 9, 2026
8560a74
Fix blocking review findings
BboyAkers Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions .github/workflows/integration-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
name: Integration Tests

@cb1kenobi cb1kenobi Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Low: no explicit permissions block

The workflow doesn't declare permissions:, so the GITHUB_TOKEN inherits whatever the repo/org default grants (which can be read-write). This workflow only checks out code and runs tests — it never comments on PRs or pushes — so it needs nothing beyond read access.

Suggested fix: add near the top of the file:

permissions:
  contents: read

—
Generated by Barber AI


on:
push:
branches: [main]
pull_request:
workflow_dispatch:
inputs:
node-version:
description: 'Node.js version'
required: true
type: choice
default: 'all'
options:
- 'all'
- '22'
- '24'
- '26'

jobs:
generate-node-version-matrix:
name: Generate Node Version Matrix
runs-on: ubuntu-latest
outputs:
node-versions: ${{ steps.set-node-versions.outputs.node-versions }}

steps:
- name: Checkout code

@cb1kenobi cb1kenobi Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: unneeded checkout in generate-node-version-matrix

This job only derives the node-versions output from the workflow_dispatch input via a shell one-liner — it never reads repository content, so the checkout step does nothing useful.

Suggested fix: drop this step (and the actions/checkout dependency) from the job.

—
Generated by Barber AI

uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3

- name: Set Node versions
id: set-node-versions
run: |
if [ "${{ github.event.inputs.node-version }}" == "all" ] || [ -z "${{ github.event.inputs.node-version }}" ]; then
echo "node-versions=[22, 24, 26]" >> $GITHUB_OUTPUT
else
echo "node-versions=[${{ github.event.inputs.node-version }}]" >> $GITHUB_OUTPUT
fi

integration-tests:
name: Integration Tests (Node ${{ matrix.node-version }})
needs: [generate-node-version-matrix]
runs-on: ubuntu-latest

@BboyAkers BboyAkers Jun 11, 2026

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.

Missing job-level timeout — the integration-tests job has no timeout-minutes. GitHub's default is 6 hours. The startupTimeoutMs: 60000 inside the test guards Harper's startup, but if a test or Harper teardown hangs afterwards the job will burn the full 6-hour budget before failing.

Suggest adding timeout-minutes: 15 (or whatever fits your expected run time) at the job level:

  integration-tests:
    name: Integration Tests (Node ${{ matrix.node-version }})
    needs: [generate-node-version-matrix]
    runs-on: ubuntu-latest
    timeout-minutes: 15

timeout-minutes: 15

strategy:
fail-fast: false
matrix:
node-version: ${{ fromJSON(needs.generate-node-version-matrix.outputs.node-versions) }}

steps:
- name: Checkout code
uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3

@BboyAkers BboyAkers Jun 11, 2026

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.

No npm cache configured — the setup-node step is missing cache: 'npm'. With three matrix legs (Node 22, 24, 26) each doing a full npm ci, every run re-downloads the full dependency tree. Adding caching typically cuts install time from ~60s to ~5s per leg.

      - name: Set up Node.js
        uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
        with:
          node-version: ${{ matrix.node-version }}
          cache: 'npm'

- name: Set up Node.js
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version: ${{ matrix.node-version }}

@cb1kenobi cb1kenobi Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Low: no npm cache configured

All three matrix legs (Node 22/24/26) run a full npm ci with no dependency cache, so each leg re-downloads the entire tree from scratch. actions/setup-node supports built-in caching, and the repo already has a package-lock.json for it to key off.

Suggested fix:

Suggested change
node-version: ${{ matrix.node-version }}
node-version: ${{ matrix.node-version }}
cache: 'npm'

—
Generated by Barber AI


- name: Install dependencies
run: npm ci

- name: Run integration tests
run: npm run test:integration
env:
HARPER_INTEGRATION_TEST_LOG_DIR: /tmp/harper-test-logs
FORCE_COLOR: '1'

- name: Upload Harper logs on failure
if: failure()
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
with:
name: harper-logs-node-${{ matrix.node-version }}
path: /tmp/harper-test-logs/
retention-days: 7
7 changes: 6 additions & 1 deletion integrationTests/app.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { suite, test, before, after } from 'node:test';
import { strictEqual, ok } from 'node:assert/strict';
import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing';
import { fileURLToPath } from 'node:url';
import { dirname, resolve } from 'node:path';

const FIXTURE_PATH = fileURLToPath(new URL('../', import.meta.url));

Expand All @@ -26,7 +27,11 @@ function authFetch(

void suite('agent-example-harper loads', (ctx: ContextWithHarper) => {
before(async () => {
await setupHarperWithFixture(ctx, FIXTURE_PATH, { startupTimeoutMs: 60000 });
// harper's exports map only exposes ".", so resolving 'harper/dist/bin/harper.js'
// (the harness's default auto-resolution) throws ERR_PACKAGE_PATH_NOT_EXPORTED. Resolve the CLI
// from harper's exported main entry and pass it explicitly via the harness escape hatch.
const harperBinPath = resolve(dirname(fileURLToPath(import.meta.resolve('harper'))), 'bin/harper.js');

@cb1kenobi cb1kenobi Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Low: harperBinPath derivation assumes harper's internal dist/ layout

resolve(dirname(fileURLToPath(import.meta.resolve('harper'))), 'bin/harper.js') assumes harper's exported main entry (dist/index.js) and its CLI (dist/bin/harper.js) stay siblings under the same dist/ directory — verified true for harper@5.0.11, but nothing pins that relationship. If a future harper release restructures dist/, this resolves to a path that doesn't exist; the harness's existsSync check in getHarperScript() will catch it and throw a clear error rather than silently running the wrong binary, so the failure mode is safe, but it's still a coupling to an unversioned internal detail.

Suggested fix: no code change needed now, but consider a one-line comment linking to the upstream tracking issue (mentioned in the PR description — harper should export its bin path) so future readers know this is a deliberate, tracked workaround rather than an oversight.

—
Generated by Barber AI

await setupHarperWithFixture(ctx, FIXTURE_PATH, { startupTimeoutMs: 60000, harperBinPath });
});

after(async () => {
Expand Down