-
Notifications
You must be signed in to change notification settings - Fork 1
Upgrade Harper to v5 #4
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| name: Integration Tests | ||
|
|
||
| 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 | ||
| uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 | ||
|
|
||
| - name: Set Node versions | ||
| id: set-node-versions | ||
| env: | ||
| NODE_VER: ${{ github.event.inputs.node-version }} | ||
| run: | | ||
| if [ "$NODE_VER" == "all" ] || [ -z "$NODE_VER" ]; then | ||
| echo "node-versions=[22, 24, 26]" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "node-versions=[$NODE_VER]" >> $GITHUB_OUTPUT | ||
| fi | ||
|
|
||
| integration-tests: | ||
| name: Integration Tests (Node ${{ matrix.node-version }}) | ||
| needs: [generate-node-version-matrix] | ||
| runs-on: ubuntu-latest | ||
|
|
||
| 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 | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 | ||
| with: | ||
| node-version: ${{ matrix.node-version }} | ||
|
|
||
| - 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| import { suite, test, before, after } from 'node:test'; | ||
| import { strictEqual, ok, deepStrictEqual } from 'node:assert/strict'; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cleanup:
// Before
import { strictEqual, ok, deepStrictEqual } from "node:assert/strict";
// After
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'; | ||
| 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'); | ||
|
|
||
| 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 }, | ||
| }); | ||
| } | ||
|
|
||
| function mcpPost(ctx: ContextWithHarper, body: Record<string, unknown>) { | ||
| return authFetch(ctx, '/mcp', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
| } | ||
|
|
||
| void suite('MCP Server', (ctx: ContextWithHarper) => { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: In
Fix: Use limit = Number.isFinite(parsed) ? parsed : undefined;or inline at the call site: |
||
| before(async () => { | ||
| await setupHarperWithFixture(ctx, FIXTURE_PATH, { harperBinPath }); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| await teardownHarper(ctx); | ||
| }); | ||
|
|
||
| void test('Harper starts successfully', async () => { | ||
| const res = await authFetch(ctx, '/'); | ||
| ok([200, 400, 404].includes(res.status), `Unexpected status ${res.status}`); | ||
| }); | ||
|
|
||
| void test('POST /mcp with resources/list returns resources array', async () => { | ||
| const res = await mcpPost(ctx, { | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| method: 'resources/list', | ||
| }); | ||
| strictEqual(res.status, 200); | ||
| const body = (await res.json()) as Record<string, unknown>; | ||
| strictEqual(body.jsonrpc, '2.0'); | ||
| strictEqual(body.id, 1); | ||
| ok(body.result !== undefined, 'expected result field'); | ||
| const result = body.result as Record<string, unknown>; | ||
| ok(Array.isArray(result.resources), 'expected resources array'); | ||
| }); | ||
|
|
||
| void test('POST /mcp with resources/read returns error when uri missing', async () => { | ||
| const res = await mcpPost(ctx, { | ||
| jsonrpc: '2.0', | ||
| id: 2, | ||
| method: 'resources/read', | ||
| params: {}, | ||
| }); | ||
| strictEqual(res.status, 200); | ||
| const body = (await res.json()) as Record<string, unknown>; | ||
| strictEqual(body.jsonrpc, '2.0'); | ||
| ok(body.error !== undefined, 'expected error for missing uri'); | ||
| const error = body.error as Record<string, unknown>; | ||
| strictEqual(error.code, -32602); | ||
| }); | ||
|
|
||
| void test('POST /mcp with unknown method returns method-not-found error', async () => { | ||
| const res = await mcpPost(ctx, { | ||
| jsonrpc: '2.0', | ||
| id: 3, | ||
| method: 'tools/call', | ||
| }); | ||
| strictEqual(res.status, 200); | ||
| const body = (await res.json()) as Record<string, unknown>; | ||
| strictEqual(body.jsonrpc, '2.0'); | ||
| ok(body.error !== undefined, 'expected error for unknown method'); | ||
| const error = body.error as Record<string, unknown>; | ||
| strictEqual(error.code, -32601); | ||
| }); | ||
|
|
||
| void test('POST /mcp with resources/read and valid uri returns contents', async () => { | ||
| // Provide a URI that points to the mcp endpoint itself (any valid resource path) | ||
| // The server returns empty contents for an unmatched path — that is valid behavior. | ||
| const res = await mcpPost(ctx, { | ||
| jsonrpc: '2.0', | ||
| id: 4, | ||
| method: 'resources/read', | ||
| params: { uri: `${ctx.harper.httpURL}/nonexistent` }, | ||
| }); | ||
| strictEqual(res.status, 200); | ||
| const body = (await res.json()) as Record<string, unknown>; | ||
| strictEqual(body.jsonrpc, '2.0'); | ||
| // Either a result with contents or an error — both are valid outcomes | ||
| ok(body.result !== undefined || body.error !== undefined, 'expected result or error'); | ||
| if (body.result) { | ||
| const result = body.result as Record<string, unknown>; | ||
| ok(Array.isArray(result.contents), 'expected contents array'); | ||
| } | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading action version comments (security/audit concern)
All three pinned actions have version comments that do not match any released version:
actions/checkout# v6.0.3actions/setup-node# v6.4.0actions/upload-artifact# v7.0.1Pinning to a commit hash is the right security practice — the hash is what GitHub actually runs. But when the human-readable version comment is wrong by 2–3 major versions, auditors relying on the comment to verify the pin will draw false conclusions about what is deployed. Please verify each hash against the correct tag and update the comments to match:
# Example: verify checkout hash gh api repos/actions/checkout/git/ref/tags/v4.2.2 --jq .object.sha