test(integration): wire up Harper v5 integration test harness#67
Merged
test(integration): wire up Harper v5 integration test harness#67
Conversation
Adds @harperfast/integration-testing-based integration tests that boot a real Harper child process with the OAuth plugin installed via npm pack. Mirrors the HarperFast/nextjs PR #40 pattern. First test asserts ${VAR} substitution in provider config reaches the authorization redirect's client_id. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewed; no blockers found. |
Member
Author
|
@claude if there are no blockers found, why is ci red? |
|
Reviewed; no blockers found. |
Pin harper to 5.0.7 (was ^5.0.0 → 5.0.2 in lockfile). Importing real harper 5.0.7 eagerly opens the system RocksDB (regressed in dist/resources/databases.js where `RocksDatabase.open(store)` became `new RocksIndexStore().open()` — opens synchronously). Under unit-test parallelism the eager open contends for the LOCK and surfaces a "Cannot read properties of undefined (reading 'localhost')" async rejection that Node's test runner reports as a flaky failure. Fix: mirror the existing Bun preload mock for Node — add test/helpers/harper-mock.mjs (a `module.register` ESM loader) and wire it into every unit-test script via `--import`. Integration tests do not load it; they need the real harper module. Also: - Drop Node 20 (out of LTS): remove from PR Checks matrix, drop the test:node-twenty script, bump engines.node to >=22, bump @types/node to ^22. - bunfig.toml: scope `bun test` to ./test so it doesn't try to run the integration tests under the bun runner. - .prettierignore: exclude .claude/ and integrationTests/fixtures/**/ node_modules/. - integrationTests/env-var-substitution.test.ts: prettier-formatted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kriszyp
pushed a commit
to HarperFast/harper
that referenced
this pull request
May 5, 2026
…collision Background: PR #432's `--edit-last` mechanism filters by authenticated identity (`claude[bot]`) only. After a `@claude` mention, the most recent `claude[bot]` comment is the mention response — so the next review run's `--edit-last` clobbers it instead of editing the prior review. Observed on HarperFast/oauth#67. Fix — three coordinated changes: 1. **Sentinel marker.** Every review comment body now MUST begin with the literal first line `<!-- claude-review:v1 -->`. This is what distinguishes review comments from mention responses, which never carry the marker. 2. **Workflow lookup step + agent-side targeted edit.** A new `Find prior review comment` step runs before the Claude review step. It queries the issue-comments REST endpoint, filters for `claude[bot]` + marker-prefixed bodies, and exposes the integer database ID (or empty) as `steps.prior_review.outputs.id`. The agent reads this via the prompt's new `PRIOR_REVIEW_COMMENT_ID` field and: - if non-empty, edits via `gh api -X PATCH .../issues/comments/<id>` - otherwise posts fresh via `gh pr comment <N>` Either way, the body starts with the marker. Adds `Bash(gh api:*)` to `--allowedTools`. Token only carries `pull-requests: write` / `contents: read` / `id-token: write`, so the broader API surface is contained to the same single PR. 3. **Log step now filters by marker too.** Same logic that the review uses — marker'd comment via raw API, claude[bot] login. Also switched the staleness guard from `created_at` to `updated_at`, since edit-in-place leaves `created_at` frozen at the original post time and we need the most recent activity to judge whether the body is fresh-this-run or stale. Existing un-marker'd review comments on active PRs will be orphaned (the next review run finds no prior, posts a marker'd fresh one). Acceptable; old comments stay as historical record. Mirror to oauth follows in a separate PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 5.0.7-regression framing in the mock's docblock and CLAUDE.md was wrong (per harper team feedback): RocksDB simply doesn't support multi-process read-write access to the same database. The 5.0.2 → 5.0.7 change in databases.js only changed how the contention surfaces, not whether it happens. Reframe accordingly and point at Chris's in-flight read-only RocksDB mode as the path to dropping this mock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewed; no blockers found. CI is green on all checks (Integration Tests Node 22/24, Lint/Build/Test 22/24, Bun) — the red CI from the prior round was resolved by the third commit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@harperfast/integration-testing-based integration tests that boot a real Harper v5 child process with the OAuth plugin installed vianpm pack. Mirrors the HarperFast/nextjs PR #40 pattern.${VAR}substitution in provider config reaches the authorization redirect'sclient_id(regression guard for Slack false-alarm thread)..github/workflows/integration-tests.yml(Node 22/24 matrix, log artifact upload on failure, SHA-pinned actions).Notes
npm pack+ tarball install (scripts/install-fixtures.js), notfile:symlinks. The symlink pattern trips Harper v5's VM sandbox withCan not load module outside of allowed path. Tarball install creates real files undernode_modules, mirroring how real users install the plugin.applications.{lockdown,moduleLoader,dependencyLoader}overrides are intentionally not set — the test runs under default v5 sandboxing.config.yamlincludesrest: true. Harper only falls back toDEFAULT_CONFIG(which setsrest: true) when noconfig.yamlis present at all; partial configs do not get defaults merged.setup-loopback.sh(macOS-only, targetslo0) is skipped on CI.Test plan
npm install && npm run install:fixtures && npm run test:integrationpasses🤖 Generated with Claude Code