From 78538ed88e108dd721f91c73f57184cd7cbb6870 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 26 May 2026 10:27:25 -0700 Subject: [PATCH 1/5] =?UTF-8?q?WIP:=20scripts/v4-routing-repro.mjs=20?= =?UTF-8?q?=E2=80=94=20v4=20counterpart=20of=20path-segment-routing=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lightweight repro harness for the Studio/CM Okta SSO symptom (Dawson, 2026-05-22) on the v1.x stack: spawns harperdb v4 against a temp fixture with two providers (decoy named "oauth", tenant named "oac-tenant-acme") and asserts that /oauth/oac-tenant-acme/login redirects to the tenant — not the decoy. NOT YET WORKING. harperdb v4's CLI reads its root path from ~/.harperdb/hdb_boot_properties.file (written by `harperdb install`) and doesn't appear to honor a ROOTPATH/HDB_ROOT env override. Two remaining paths: script a `harperdb install` against a temp dir with atomic save/restore of the boot-properties file, or identify the correct HARPER_
_ env vars v4 accepts (analogous to v5's HARPER_SET_CONFIG). See file header for current state. The counterpart test on main (Harper 5.0.7) is integrationTests/path-segment-routing.test.ts and currently passes — i.e. parseRoute on v5 correctly extracts the URL path segment and Dawson's diagnosis is not reproducible on that stack. parseRoute on v1.x (v1.4.0) is byte-identical to main; if the symptom is real on Dawson's stack, it would have to come from a Harper v4 behavior difference (target.id shape), not the plugin code. Co-Authored-By: Claude Opus 4.7 (1M context) --- scripts/v4-routing-repro.mjs | 236 +++++++++++++++++++++++++++++++++++ 1 file changed, 236 insertions(+) create mode 100644 scripts/v4-routing-repro.mjs diff --git a/scripts/v4-routing-repro.mjs b/scripts/v4-routing-repro.mjs new file mode 100644 index 0000000..1329785 --- /dev/null +++ b/scripts/v4-routing-repro.mjs @@ -0,0 +1,236 @@ +#!/usr/bin/env node +/** + * v4-routing-repro.mjs — lightweight repro for the symptom Dawson reported + * on 2026-05-22 (CM/Studio Okta SSO): + * + * "providerName is 'oauth', not the {configId} described, that isn't passed" + * + * Counterpart of integrationTests/path-segment-routing.test.ts on `main` + * (Harper v5), built for the v1.x line where CM consumes us against + * harperdb ^4.7.28. + * + * Spawns harperdb v4 against a temp fixture that configures two providers — + * one literally named "oauth" (decoy) and one named "oac-tenant-acme" with + * a distinctive client_id — then fires /oauth/oac-tenant-acme/login and + * inspects the resulting redirect. + * + * Expected outcomes once boot works: + * - Redirect to tenant.test/authorize with the tenant client_id → parseRoute + * correctly extracted the URL path segment. Dawson's diagnosis would not + * reproduce on this stack. + * - Redirect to decoy.test/authorize → parseRoute extracted the literal + * "oauth" prefix as providerName. Bug confirmed on this stack. + * + * STATUS — WIP, does NOT currently boot harperdb v4. + * + * harperdb v4 reads its root path from ~/.harperdb/hdb_boot_properties.file + * (written by `harperdb install`). The CLI doesn't accept ROOTPATH / + * HDB_ROOT env vars to override it, and any non-default install would + * overwrite Nathan's existing /Users/nathan/harper pointer. Need either: + * (a) scripted `harperdb install` to a temp dir + atomic save/restore of + * the boot-properties file, OR + * (b) find the correct env var(s) harperdb v4 honors (likely + * HARPER_
_ like Harper v5's HARPER_SET_CONFIG; not yet + * verified for v4) + * + * Run manually (once it works): + * npm run build && node scripts/v4-routing-repro.mjs + * + * Set KEEP_TEMP=1 to preserve the temp dir for inspection. + */ +import { mkdtempSync, writeFileSync, rmSync, readFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { spawn, spawnSync } from 'node:child_process'; + +const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '..'); +const harperBin = join(repoRoot, 'node_modules', '.bin', 'harperdb'); + +const TENANT_CLIENT_ID = 'tenant-acme-client-id'; +const DECOY_CLIENT_ID = 'decoy-oauth-client-id'; + +const CONFIG_YAML = ` +rest: true + +'@harperfast/oauth': + package: '@harperfast/oauth' + providers: + oauth: + provider: generic + clientId: ${DECOY_CLIENT_ID} + clientSecret: decoy-secret + authorizationUrl: 'http://decoy.test/authorize' + tokenUrl: 'http://decoy.test/token' + userInfoUrl: 'http://decoy.test/userinfo' + oac-tenant-acme: + provider: generic + clientId: ${TENANT_CLIENT_ID} + clientSecret: tenant-secret + authorizationUrl: 'http://tenant.test/authorize' + tokenUrl: 'http://tenant.test/token' + userInfoUrl: 'http://tenant.test/userinfo' +`.trimStart(); + +const PACKAGE_JSON = JSON.stringify( + { + name: 'oauth-v4-routing-repro', + private: true, + type: 'module', + }, + null, + 2 +); + +function run(cmd, args, opts = {}) { + const result = spawnSync(cmd, args, { stdio: 'inherit', ...opts }); + if (result.status !== 0) { + throw new Error(`${cmd} ${args.join(' ')} exited ${result.status}`); + } +} + +function log(...args) { + console.log('[v4-repro]', ...args); +} + +async function waitForReady(harperProc, timeoutMs) { + const deadline = Date.now() + timeoutMs; + return new Promise((resolve, reject) => { + let buf = ''; + const onData = (chunk) => { + const s = String(chunk); + buf += s; + process.stdout.write(s); + if (/successfully started/i.test(buf)) { + cleanup(); + resolve(); + } + }; + const onExit = (code) => { + cleanup(); + reject(new Error(`harperdb exited prematurely with code ${code}`)); + }; + const onTimeout = () => { + cleanup(); + reject(new Error(`harperdb did not become ready within ${timeoutMs}ms`)); + }; + const timer = setTimeout(onTimeout, deadline - Date.now()); + const cleanup = () => { + clearTimeout(timer); + harperProc.stdout?.off('data', onData); + harperProc.stderr?.off('data', onData); + harperProc.off('exit', onExit); + }; + harperProc.stdout?.on('data', onData); + harperProc.stderr?.on('data', onData); + harperProc.once('exit', onExit); + }); +} + +async function main() { + const tempRoot = mkdtempSync(join(tmpdir(), 'oauth-v4-repro-')); + const fixtureDir = join(tempRoot, 'app'); + const hdbRoot = join(tempRoot, 'hdb-root'); + log(`temp root: ${tempRoot}`); + + let harperProc = null; + let exitCode = 1; + + try { + // 1. Build the plugin (so dist/ is current) + log('building plugin...'); + run('npm', ['run', 'build'], { cwd: repoRoot }); + + // 2. npm pack the local plugin into the temp dir + log('packing local plugin...'); + const packResult = spawnSync('npm', ['pack', '--pack-destination', tempRoot, '--json'], { + cwd: repoRoot, + encoding: 'utf8', + }); + if (packResult.status !== 0) { + console.error(packResult.stderr); + throw new Error(`npm pack failed (exit ${packResult.status})`); + } + const tarballName = JSON.parse(packResult.stdout)[0].filename; + const tarballPath = join(tempRoot, tarballName); + + // 3. Set up the fixture + log(`writing fixture to ${fixtureDir}...`); + spawnSync('mkdir', ['-p', fixtureDir], { stdio: 'inherit' }); + writeFileSync(join(fixtureDir, 'config.yaml'), CONFIG_YAML); + writeFileSync(join(fixtureDir, 'package.json'), PACKAGE_JSON); + + log('installing plugin into fixture...'); + run('npm', ['install', '--no-save', '--no-audit', '--no-fund', tarballPath], { cwd: fixtureDir }); + + // 4. Spawn harperdb dev + // Using HDB_ROOT to direct harperdb's data dir to a temp location so we + // don't touch ~/hdb. Plain HTTP on 9926 by default (securePort overrides + // but dev mode usually serves plain HTTP for ease of testing). + log(`spawning: ${harperBin} dev ${fixtureDir}`); + harperProc = spawn(harperBin, ['dev', fixtureDir], { + env: { + ...process.env, + HDB_ROOT: hdbRoot, + ROOTPATH: hdbRoot, + }, + }); + + log('waiting for ready...'); + await waitForReady(harperProc, 60_000); + + // 5. Fire the request and check the redirect + // Try plain HTTP on 9926 first; harperdb dev usually exposes it. + const url = 'http://localhost:9926/oauth/oac-tenant-acme/login'; + log(`fetching ${url}`); + const response = await fetch(url, { redirect: 'manual' }); + log(`status: ${response.status}`); + const location = response.headers.get('location'); + log(`location: ${location}`); + + if (response.status !== 302 || !location) { + throw new Error(`expected 302 redirect, got ${response.status} (location: ${location})`); + } + + const redirectUrl = new URL(location); + const target = redirectUrl.origin + redirectUrl.pathname; + const clientId = redirectUrl.searchParams.get('client_id'); + + log(`redirect target: ${target}`); + log(`client_id: ${clientId}`); + + if (target === 'http://tenant.test/authorize' && clientId === TENANT_CLIENT_ID) { + log('PASS: routed to tenant provider — parseRoute extracted the URL path segment'); + exitCode = 0; + } else if (target === 'http://decoy.test/authorize' && clientId === DECOY_CLIENT_ID) { + log('FAIL: routed to DECOY provider — parseRoute extracted literal "oauth" segment (bug confirmed on this stack)'); + exitCode = 1; + } else { + log(`UNEXPECTED: target=${target} client_id=${clientId}`); + exitCode = 1; + } + } catch (err) { + log('error:', err.message); + exitCode = 1; + } finally { + if (harperProc && harperProc.exitCode === null) { + log('killing harperdb...'); + harperProc.kill('SIGINT'); + await new Promise((r) => setTimeout(r, 1000)); + if (harperProc.exitCode === null) harperProc.kill('SIGKILL'); + } + if (!process.env.KEEP_TEMP) { + log(`cleaning up ${tempRoot}`); + rmSync(tempRoot, { recursive: true, force: true }); + } else { + log(`KEEP_TEMP set; leaving ${tempRoot}`); + } + } + + process.exit(exitCode); +} + +main().catch((err) => { + console.error('[v4-repro] fatal:', err); + process.exit(1); +}); From 6556aba3fa8c14037afd0adfa8706a553448b59a Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 26 May 2026 12:59:00 -0700 Subject: [PATCH 2/5] v4-routing-repro now passes on harperdb 4.7.19 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Working set of CLI args mirrors @harperfast/integration-testing@0.3.0's spawn pattern (--ROOTPATH, --HDB_ADMIN_*, --HTTP_PORT, --DEFAULTS_MODE, --TC_AGREEMENT, --REPLICATION_HOSTNAME), so harperdb installs into a temp dir and binds on a non-default port without prompting. Result: /oauth/oac-tenant-acme/login redirects to tenant.test/authorize with the tenant client_id (NOT the decoy provider literally named "oauth"). parseRoute on the v1.4.0 plugin source + harperdb 4.7.19 correctly extracts the URL path segment — same as Harper v5. Dawson's "providerName is 'oauth'" diagnosis does not reproduce on either stack. Atomic save/restore of ~/.harperdb/hdb_boot_properties.file: harperdb's installer rewrites that file to point at the temp ROOTPATH. The script now backs up the original before launch and restores it in finally (plus SIGINT/SIGTERM handlers for unclean exits), so an existing local harperdb install survives repeated runs intact. Co-Authored-By: Claude Opus 4.7 (1M context) --- scripts/v4-routing-repro.mjs | 145 +++++++++++++++++++++++++++-------- 1 file changed, 112 insertions(+), 33 deletions(-) diff --git a/scripts/v4-routing-repro.mjs b/scripts/v4-routing-repro.mjs index 1329785..005c27b 100644 --- a/scripts/v4-routing-repro.mjs +++ b/scripts/v4-routing-repro.mjs @@ -1,6 +1,6 @@ #!/usr/bin/env node /** - * v4-routing-repro.mjs — lightweight repro for the symptom Dawson reported + * v4-routing-repro.mjs — regression guard for the symptom Dawson reported * on 2026-05-22 (CM/Studio Okta SSO): * * "providerName is 'oauth', not the {configId} described, that isn't passed" @@ -14,36 +14,34 @@ * a distinctive client_id — then fires /oauth/oac-tenant-acme/login and * inspects the resulting redirect. * - * Expected outcomes once boot works: + * Outcomes: * - Redirect to tenant.test/authorize with the tenant client_id → parseRoute - * correctly extracted the URL path segment. Dawson's diagnosis would not + * correctly extracted the URL path segment. This is the current behavior + * on Harper v4.7.19 + v1.4.0 plugin source — Dawson's diagnosis does not * reproduce on this stack. * - Redirect to decoy.test/authorize → parseRoute extracted the literal - * "oauth" prefix as providerName. Bug confirmed on this stack. + * "oauth" prefix as providerName. Bug confirmed (regression). * - * STATUS — WIP, does NOT currently boot harperdb v4. + * Side effects — harperdb's installer rewrites ~/.harperdb/hdb_boot_properties.file + * to point at the temp ROOTPATH. The script saves the original contents (if any) + * before launch and restores them in a finally block, so an existing local + * `harperdb` install is left intact across runs. If the script is killed + * uncleanly, manually restore the file from ${BOOT_BACKUP_PATH}. * - * harperdb v4 reads its root path from ~/.harperdb/hdb_boot_properties.file - * (written by `harperdb install`). The CLI doesn't accept ROOTPATH / - * HDB_ROOT env vars to override it, and any non-default install would - * overwrite Nathan's existing /Users/nathan/harper pointer. Need either: - * (a) scripted `harperdb install` to a temp dir + atomic save/restore of - * the boot-properties file, OR - * (b) find the correct env var(s) harperdb v4 honors (likely - * HARPER_
_ like Harper v5's HARPER_SET_CONFIG; not yet - * verified for v4) - * - * Run manually (once it works): + * Not wired into CI. Run manually: * npm run build && node scripts/v4-routing-repro.mjs * * Set KEEP_TEMP=1 to preserve the temp dir for inspection. */ -import { mkdtempSync, writeFileSync, rmSync, readFileSync } from 'node:fs'; -import { tmpdir } from 'node:os'; +import { mkdtempSync, writeFileSync, rmSync, readFileSync, existsSync, copyFileSync } from 'node:fs'; +import { tmpdir, homedir } from 'node:os'; import { join, dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { spawn, spawnSync } from 'node:child_process'; +const BOOT_PROPS_PATH = join(homedir(), '.harperdb', 'hdb_boot_properties.file'); +const BOOT_BACKUP_PATH = join(tmpdir(), 'oauth-v4-repro-hdb_boot_properties.file.bak'); + const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '..'); const harperBin = join(repoRoot, 'node_modules', '.bin', 'harperdb'); @@ -127,12 +125,46 @@ async function waitForReady(harperProc, timeoutMs) { }); } +function backupBootProps() { + if (existsSync(BOOT_PROPS_PATH)) { + copyFileSync(BOOT_PROPS_PATH, BOOT_BACKUP_PATH); + log(`saved boot props to ${BOOT_BACKUP_PATH}`); + return 'existed'; + } + log('no existing boot props file to back up'); + return 'absent'; +} + +function restoreBootProps(state) { + try { + if (state === 'existed') { + if (existsSync(BOOT_BACKUP_PATH)) { + copyFileSync(BOOT_BACKUP_PATH, BOOT_PROPS_PATH); + rmSync(BOOT_BACKUP_PATH, { force: true }); + log(`restored boot props from backup`); + } else { + log(`WARNING: backup at ${BOOT_BACKUP_PATH} missing — leaving boot props as-is`); + } + } else if (state === 'absent') { + rmSync(BOOT_PROPS_PATH, { force: true }); + log(`removed boot props (none existed before)`); + } + } catch (err) { + log(`WARNING: failed to restore boot props: ${err.message}. Original backup at ${BOOT_BACKUP_PATH}`); + } +} + async function main() { const tempRoot = mkdtempSync(join(tmpdir(), 'oauth-v4-repro-')); const fixtureDir = join(tempRoot, 'app'); const hdbRoot = join(tempRoot, 'hdb-root'); log(`temp root: ${tempRoot}`); + // Save the current boot props before harperdb's installer rewrites them. + // Done outside the try so the backup is in place if anything below throws + // before harperdb spawns. + const bootState = backupBootProps(); + let harperProc = null; let exitCode = 1; @@ -163,25 +195,47 @@ async function main() { log('installing plugin into fixture...'); run('npm', ['install', '--no-save', '--no-audit', '--no-fund', tarballPath], { cwd: fixtureDir }); - // 4. Spawn harperdb dev - // Using HDB_ROOT to direct harperdb's data dir to a temp location so we - // don't touch ~/hdb. Plain HTTP on 9926 by default (securePort overrides - // but dev mode usually serves plain HTTP for ease of testing). - log(`spawning: ${harperBin} dev ${fixtureDir}`); - harperProc = spawn(harperBin, ['dev', fixtureDir], { - env: { - ...process.env, - HDB_ROOT: hdbRoot, - ROOTPATH: hdbRoot, - }, - }); + // 4. Spawn harperdb with CLI-arg config — modeled on + // @harperfast/integration-testing@0.3.0 (v5). Same argument names + // exist in v4's bundled binary (ROOTPATH, HDB_ADMIN_*, HTTP_PORT, + // OPERATIONSAPI_*, etc.). + // + // Also point Harper at the fixture directory as its componentsRoot + // so the OAuth plugin loads from there (the fixture has the plugin + // installed via npm pack above). + const httpPort = 19926; + const opsPort = 19925; + const hostname = '127.0.0.1'; + const args = [ + `--ROOTPATH=${hdbRoot}`, + `--TC_AGREEMENT=yes`, + `--HDB_ADMIN_USERNAME=admin`, + `--HDB_ADMIN_PASSWORD=Abc1234!`, + `--DEFAULTS_MODE=dev`, + `--REPLICATION_HOSTNAME=localhost`, + `--HTTP_PORT=${hostname}:${httpPort}`, + `--OPERATIONSAPI_NETWORK_PORT=${hostname}:${opsPort}`, + `--NODE_HOSTNAME=${hostname}`, + `--THREADS_COUNT=1`, + `--LOGGING_LEVEL=debug`, + `--LOGGING_STDSTREAMS=true`, + `--CLUSTERING_ENABLED=false`, + `--COMPONENTSROOT=${tempRoot}/components`, + ]; + + // Move the fixture into a `components/` subdir so harperdb finds it + // as a component by directory name. + run('mkdir', ['-p', join(tempRoot, 'components')]); + run('cp', ['-r', fixtureDir, join(tempRoot, 'components', 'app')]); + + log(`spawning: ${harperBin} ${args.join(' ')}`); + harperProc = spawn(harperBin, args); log('waiting for ready...'); await waitForReady(harperProc, 60_000); - // 5. Fire the request and check the redirect - // Try plain HTTP on 9926 first; harperdb dev usually exposes it. - const url = 'http://localhost:9926/oauth/oac-tenant-acme/login'; + // 5. Fire the request and check the redirect. + const url = `http://${hostname}:${httpPort}/oauth/oac-tenant-acme/login`; log(`fetching ${url}`); const response = await fetch(url, { redirect: 'manual' }); log(`status: ${response.status}`); @@ -219,6 +273,7 @@ async function main() { await new Promise((r) => setTimeout(r, 1000)); if (harperProc.exitCode === null) harperProc.kill('SIGKILL'); } + restoreBootProps(bootState); if (!process.env.KEEP_TEMP) { log(`cleaning up ${tempRoot}`); rmSync(tempRoot, { recursive: true, force: true }); @@ -230,6 +285,30 @@ async function main() { process.exit(exitCode); } +// Restore boot props on unclean exits too — Ctrl+C, kill, uncaught exceptions. +let signaled = false; +function emergencyRestore() { + if (signaled) return; + signaled = true; + try { + if (existsSync(BOOT_BACKUP_PATH)) { + copyFileSync(BOOT_BACKUP_PATH, BOOT_PROPS_PATH); + rmSync(BOOT_BACKUP_PATH, { force: true }); + console.log(`[v4-repro] emergency: restored boot props from ${BOOT_BACKUP_PATH}`); + } + } catch (err) { + console.error(`[v4-repro] emergency restore failed: ${err.message}`); + } +} +process.on('SIGINT', () => { + emergencyRestore(); + process.exit(130); +}); +process.on('SIGTERM', () => { + emergencyRestore(); + process.exit(143); +}); + main().catch((err) => { console.error('[v4-repro] fatal:', err); process.exit(1); From fa5eaf359d42b60351b3dc2bff9816fabb121ec1 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 26 May 2026 13:12:11 -0700 Subject: [PATCH 3/5] Drop unused readFileSync import + prettier formatting Lint failure on CI: stale import from an earlier shape of the script. Also wrapped the long FAIL log string per prettier. Co-Authored-By: Claude Opus 4.7 (1M context) --- scripts/v4-routing-repro.mjs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/v4-routing-repro.mjs b/scripts/v4-routing-repro.mjs index 005c27b..679c0c2 100644 --- a/scripts/v4-routing-repro.mjs +++ b/scripts/v4-routing-repro.mjs @@ -33,7 +33,7 @@ * * Set KEEP_TEMP=1 to preserve the temp dir for inspection. */ -import { mkdtempSync, writeFileSync, rmSync, readFileSync, existsSync, copyFileSync } from 'node:fs'; +import { mkdtempSync, writeFileSync, rmSync, existsSync, copyFileSync } from 'node:fs'; import { tmpdir, homedir } from 'node:os'; import { join, dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -257,7 +257,9 @@ async function main() { log('PASS: routed to tenant provider — parseRoute extracted the URL path segment'); exitCode = 0; } else if (target === 'http://decoy.test/authorize' && clientId === DECOY_CLIENT_ID) { - log('FAIL: routed to DECOY provider — parseRoute extracted literal "oauth" segment (bug confirmed on this stack)'); + log( + 'FAIL: routed to DECOY provider — parseRoute extracted literal "oauth" segment (bug confirmed on this stack)' + ); exitCode = 1; } else { log(`UNEXPECTED: target=${target} client_id=${clientId}`); From 9340a9830e8a3b1742d503a0f0d27d4141e839ee Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 26 May 2026 13:19:16 -0700 Subject: [PATCH 4/5] Sync CI workflows from main onto v1.x MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring the v1.x .github/workflows/ tree in line with main's so the Claude review action's "workflow file must be identical to default branch" validation gate passes for PRs targeting v1.x. The chronic mismatch was silently failing every v1.x PR's review job at the OIDC-token-exchange step. What changes for v1.x as a side effect of the harmonization: - pr-checks.yml: Node 20 dropped from the test matrix (was [20, 22, 24], now [22, 24]); test:coverage is the single coverage path (the v1.x-specific test:node-twenty fork is no longer wired into CI but the script remains in package.json). Action pins updated to current SHAs. - release.yml: setup-node SHA bumped to v6.4.0 to match main. - claude-review.yml: rewritten as a thin caller of the reusable in HarperFast/ai-review-prompts (this is what unblocks the review check). - New: claude-issue-to-pr.yml, claude-mention.yml, gemini-review.yml, validate-caller-workflows.yml — additive, parallel to main's review infrastructure. NOT synced: integration-tests.yml. That workflow runs `npm run install:fixtures && npm run test:integration`, neither of which exists on v1.x. If we want a similar CI check on this branch later it should wrap scripts/v4-routing-repro.mjs (added in this PR), which is a different shape than main's harness. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/claude-issue-to-pr.yml | 77 ++++++ .github/workflows/claude-mention.yml | 86 +++++++ .github/workflows/claude-review.yml | 236 +++++------------- .github/workflows/gemini-review.yml | 84 +++++++ .github/workflows/pr-checks.yml | 18 +- .github/workflows/release.yml | 2 +- .../workflows/validate-caller-workflows.yml | 33 +++ 7 files changed, 352 insertions(+), 184 deletions(-) create mode 100644 .github/workflows/claude-issue-to-pr.yml create mode 100644 .github/workflows/claude-mention.yml create mode 100644 .github/workflows/gemini-review.yml create mode 100644 .github/workflows/validate-caller-workflows.yml diff --git a/.github/workflows/claude-issue-to-pr.yml b/.github/workflows/claude-issue-to-pr.yml new file mode 100644 index 0000000..9bb985d --- /dev/null +++ b/.github/workflows/claude-issue-to-pr.yml @@ -0,0 +1,77 @@ +name: Claude Issue to PR + +# Thin caller of the reusable in HarperFast/ai-review-prompts. The +# single `uses:` ref pin below controls everything that moves +# together — workflow logic, auth script, agent prompt, allowed- +# labels list. Bumping the pin is the entire upgrade motion. +# +# Pre-requisites (org-level secrets, configured once on HarperFast): +# - HARPERFAST_AI_CLIENT_ID +# - HARPERFAST_AI_APP_PRIVATE_KEY +# +# Plus the per-repo / inherited: +# - ANTHROPIC_API_KEY (required) + +on: + issues: + types: [labeled] + +concurrency: + group: claude-issue-${{ github.event.issue.number }} + cancel-in-progress: false + +jobs: + work: + uses: HarperFast/ai-review-prompts/.github/workflows/_claude-issue-to-pr.yml@f6daed301f8a7ece74593506de38c6e80820b00b # main 2026-05-06 (post #11/#12/#14) + with: + # Same SHA as the `uses:` ref above. See the comment in + # claude-mention.yml for why the duplication is unavoidable. + ai-review-prompts-ref: f6daed301f8a7ece74593506de38c6e80820b00b + # Plugin repo — bun is part of the test path. + setup-bun: true + repo-specific-conventions: | + ## OAuth-specific notes + + - `config.yaml` holds the plugin entry point (`pluginModule`) + and runtime OAuth defaults. NOT doc-only — any change + there is a code/config change. + - Tests run under both Node (`npm test`) and Bun + (`bun test`); both must pass. + + ## Documentation scope boundary + + Harper maintains authoritative docs at + https://docs.harperdb.io covering core, pro, and fabric. + This repo's docs should NOT re-explain Harper mechanics + those docs already cover — they drift out of sync when the + Harper docs update. + + When writing or revising docs here: + + - Document what's specific to THIS component — env var + names, config shape, setup flow, integration API. + - For anything not component-specific (deployment mechanics, + runtime env var handling, Fabric configuration, core + Harper behavior, SQL, replication), LINK to the Harper + docs rather than re-explaining. + pre-commit-validation: | + **`config.yaml` is NOT doc-only** — it holds the plugin + entry point (`pluginModule`) and runtime OAuth defaults. + Any change touching `config.yaml` requires the full + validation path regardless of the `claude-fix:*` label. + + - `claude-fix:typo` / `claude-fix:docs` (doc-only changes + to `*.md`, `docs/**`, or `package.json` + keyword/description fields): run `npm run format:check` + and `npm run lint`. Skip `npm run build` / `npm test` / + `bun test` — they are not affected and waste turns. + - `claude-fix:deps` / `claude-fix:bug` or any change that + touches code or `config.yaml`: run + `npm run build && npm run lint && npm run format:check && npm test` + and `bun test`. Fix anything that fails. + + When in doubt, err toward the fuller validation. + secrets: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + HARPERFAST_AI_CLIENT_ID: ${{ secrets.HARPERFAST_AI_CLIENT_ID }} + HARPERFAST_AI_APP_PRIVATE_KEY: ${{ secrets.HARPERFAST_AI_APP_PRIVATE_KEY }} diff --git a/.github/workflows/claude-mention.yml b/.github/workflows/claude-mention.yml new file mode 100644 index 0000000..05190c7 --- /dev/null +++ b/.github/workflows/claude-mention.yml @@ -0,0 +1,86 @@ +name: Claude Mention Handler + +# Thin caller of the reusable in HarperFast/ai-review-prompts. The +# single `uses:` ref pin below controls everything that moves +# together — workflow logic, parse + auth scripts, agent prompt. +# Bumping the pin is the entire upgrade motion. +# +# Pre-requisites (org-level secrets, configured once on HarperFast): +# - HARPERFAST_AI_CLIENT_ID (the App's Client ID, like Iv23li…) +# - HARPERFAST_AI_APP_PRIVATE_KEY (.pem file contents) +# +# Plus the per-repo / inherited: +# - ANTHROPIC_API_KEY (required) + +on: + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] + +concurrency: + group: claude-mention-${{ github.event.issue.number || github.event.pull_request.number }} + cancel-in-progress: false + +jobs: + mention: + uses: HarperFast/ai-review-prompts/.github/workflows/_claude-mention.yml@f6daed301f8a7ece74593506de38c6e80820b00b # main 2026-05-06 (post #11/#12/#14) + with: + # Same SHA as the `uses:` ref above. The reusable uses this to + # check out HarperFast/ai-review-prompts (parse + auth scripts) + # at the same ref as the workflow logic itself. + # + # The duplication is unavoidable: reusable workflows can't + # introspect their own ref (`github.workflow_ref` resolves to + # the CALLER's ref in `workflow_call` context), and `uses: …@` + # is parsed literally so we can't interpolate a variable. + ai-review-prompts-ref: f6daed301f8a7ece74593506de38c6e80820b00b + # Plugin repo — opt into bun setup so the agent can run + # `bun test` and `bun run …` for repo-specific scripts. + setup-bun: true + repo-specific-conventions: | + ## OAuth-specific notes + + - `config.yaml` holds the plugin entry point (`pluginModule`) + and runtime OAuth defaults. NOT doc-only — any change there + is a code/config change requiring full validation. + - Tests run under both Node (`npm test`) and Bun (`bun test`); + both must pass before opening / pushing to a PR. + + ## Documentation scope boundary + + Harper maintains authoritative docs at + https://docs.harperdb.io covering core, pro, and fabric. + This repo's docs should NOT re-explain Harper mechanics + those docs already cover — they drift out of sync when the + Harper docs update. + + When writing or revising docs here: + + - Document what's specific to THIS component — env var + names, config shape, setup flow, integration API. + - For anything not component-specific (deployment mechanics, + runtime env var handling, Fabric configuration, core + Harper behavior, SQL, replication), LINK to the Harper + docs rather than re-explaining. + pre-commit-validation: | + Scale validation to the kind of change you made: + + - Doc-only change (only `*.md`, `docs/**`, or `package.json` + keyword/description edits): run `npm run format:check` and + `npm run lint`. Do NOT run `npm run build` / `npm test` / + `bun test` — they are not affected and waste turns. + + `config.yaml` is NOT doc-only — it holds the plugin entry + point (`pluginModule`) and runtime OAuth defaults. Treat + any change there as a code/config change. + - Code or config change that affects behavior: run `npm ci` + first (deps aren't pre-installed in this workflow), then + `npm run build && npm run lint && npm run format:check && npm test` + and `bun test`. Fix anything that fails. + + When in doubt, err toward the fuller validation. + secrets: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + HARPERFAST_AI_CLIENT_ID: ${{ secrets.HARPERFAST_AI_CLIENT_ID }} + HARPERFAST_AI_APP_PRIVATE_KEY: ${{ secrets.HARPERFAST_AI_APP_PRIVATE_KEY }} diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 98b7a1a..43f66b9 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -1,8 +1,24 @@ name: Claude PR Review +# Thin caller of the reusable in HarperFast/ai-review-prompts. The single +# `uses:` ref pin below controls everything that moves together — workflow +# logic, layer files, bash scripts, auth-gate behavior. Bumping the pin +# is the entire upgrade motion. +# +# Pre-requisites (org-level secrets, configured once on HarperFast): +# - HARPERFAST_AI_CLIENT_ID (the App's Client ID, like Iv23li…) +# - HARPERFAST_AI_APP_PRIVATE_KEY (.pem file contents) +# +# Plus the per-repo / inherited: +# - ANTHROPIC_API_KEY (required) +# - AI_REVIEW_LOG_TOKEN (optional — if set, threads each run +# into a per-PR issue in HarperFast/ai-review-log) + on: pull_request: - types: [opened, synchronize, reopened] + # `labeled` admits the `claude-review` label gesture for + # bot-authored PRs (renovate, dependabot). See ai-review-prompts#38. + types: [opened, synchronize, reopened, labeled] concurrency: group: claude-review-${{ github.event.pull_request.number }} @@ -10,176 +26,54 @@ concurrency: jobs: review: - # Only review PRs authored by HarperFast org members / collaborators. - # External PRs are not auto-reviewed — a maintainer can opt one in later - # via an @claude mention (handled by a separate workflow). - if: >- - contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), - github.event.pull_request.author_association) - runs-on: ubuntu-latest - timeout-minutes: 10 + uses: HarperFast/ai-review-prompts/.github/workflows/_claude-review.yml@533f841137114315b0b4d02dabf367e376d9922e # main 2026-05-21 (post #42 — log-script counts from "N blockers found" line + tolerates markdown wrapping, fixing Claude-recap titles undercounting; _claude-review.yml itself unchanged) + # Caller-side permissions, scoped at the calling-job level (NOT + # workflow-level — that placement caps the reusable's per-job + # grants below what they need and breaks the workflow at startup; + # see ai-review-prompts#39/#40 for the incident). Union of what + # the reusable's `authorize` (`contents: read`) and `review` + # (`contents: read + pull-requests: write + id-token: write`) + # jobs declare. GitHub's rule: caller's GITHUB_TOKEN permissions + # can only be DOWNGRADED (not elevated) by the called workflow, + # so the caller must grant at least the union the reusable needs. + # Reference: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations permissions: contents: read pull-requests: write - id-token: write # required by claude-code-action even with API-key auth - - steps: - - name: Checkout - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 - with: - fetch-depth: 0 - - - name: Claude review - id: claude-review - uses: anthropics/claude-code-action@c3d45e8e941e1b2ad7b278c57482d9c5bf1f35b3 # v1.0.99 - with: - anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} - show_full_output: true # TEMP: keep on during calibration so tool denials are visible; revert once reviews run cleanly - claude_args: | - --model claude-sonnet-4-6 - --max-turns 8 - --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Read,Grep,Glob" - prompt: | - REPO: ${{ github.repository }} - PR NUMBER: ${{ github.event.pull_request.number }} - - The PR branch is already checked out in the current working directory. - - ## Tools - - For file inspection use the `Read`, `Grep`, and `Glob` tools. - Do NOT use `cat`, `head`, `tail`, `grep`, `ls`, or `find` - via Bash — those commands are not allowed and waste turns. - Do NOT run `bun test`, `npm test`, or any other script - that executes PR code — the PR's tests are already checked - separately. - - The only allowed Bash commands are: - - `gh pr view` / `gh pr diff` — inspect the PR (already run - at start, you can re-invoke if needed) - - `gh pr comment` — post the final review comment - - Read `CLAUDE.md` in the repo root first — it has the project - overview, conventions, and (importantly) a "Non-Obvious Gotchas" - section covering Resource API v2 patterns, GenericTrackedObject - spread behavior, `tsc || true` build semantics, and security - invariants. - - ## OAuth-specific things to check - - - CSRF state tokens present on every OAuth flow; 10-minute expiry - is enforced; state is single-use - - Tokens are never logged and never returned in HTTP responses - - Redirect URI validation on callback endpoints - - Provider-of-record enforcement (cross-provider CSRF protection - should redirect with error, not 403) - - Session field preservation across token refresh (`provider`, - `providerConfigId`, `providerType`) - - Resource API v2 conventions (`static loadAsInstance = false`, - instance methods) - - `GenericTrackedObject` spread gotcha — if code uses `{...obj}` - on a session field, flag it - - New endpoints include auth checks and context validation - - Path length validation (≤ 2048 chars) where user input can - reach a path - - ## Review scope and style - - Report **blocker-severity findings only**. Blockers are: - - Correctness bugs - - Security issues (token exposure, missing CSRF, auth bypass, - unvalidated redirect/path, injection) - - Broken contracts (public API behavior change without migration) - - Missing tests for new behavior or fixed bugs - - Documentation drift that would mislead integrators - - Do NOT comment on style, naming, nits, or personal preference. - Do NOT restate what the diff does. Do NOT praise. - - Cap the review at 10 findings. - - ## How to post the review - - - Use `gh pr comment` for top-level feedback (single consolidated - summary comment with all findings). - - Use `mcp__github_inline_comment__create_inline_comment` - (with `confirmed: true`) for specific code-line annotations. - - Only post GitHub comments — do NOT submit review text as SDK - messages. - - Do NOT use REQUEST_CHANGES or APPROVE during this calibration - phase. Stick to comments so the workflow never blocks or - auto-approves a merge. - - ## Output format for the top-level comment - - If you find zero blockers: post one short comment — - "No blockers found." — and stop. - - If you find blockers: post one summary comment with all findings - using this structure, one block per finding: - - ### . - - **File:** `path/to/file.ts:line` - **What:** one or two sentence description - **Why it matters:** concrete impact - **Suggested fix:** specific change, not generic advice - - No preamble. No closing summary. - - - name: Log review to ai-review-log - # Best-effort — never fail the job if logging fails - if: always() - env: - GH_TOKEN: ${{ github.token }} - AI_REVIEW_LOG_TOKEN: ${{ secrets.AI_REVIEW_LOG_TOKEN }} - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_URL: ${{ github.event.pull_request.html_url }} - REVIEW_STATUS: ${{ steps.claude-review.outcome }} - run: | - set -uo pipefail - - if [ -z "${AI_REVIEW_LOG_TOKEN:-}" ]; then - echo "::warning::AI_REVIEW_LOG_TOKEN secret not set; skipping log entry." - exit 0 - fi - - # Fetch the latest comment posted by the Claude bot on this PR. - CLAUDE_BODY=$(gh pr view "$PR_NUMBER" --json comments \ - --jq '[.comments[] | select(.author.login == "claude")] | last | .body // empty') - - if [ -z "$CLAUDE_BODY" ]; then - echo "No Claude comment found on PR #$PR_NUMBER (review_status=$REVIEW_STATUS); skipping log." - exit 0 - fi - - # Title: count findings (lines starting with `### `). "No blockers" case has none. - if printf '%s' "$CLAUDE_BODY" | grep -qi '^no blockers found'; then - TITLE="[oauth] PR #$PR_NUMBER: no blockers" - else - FINDING_COUNT=$(printf '%s\n' "$CLAUDE_BODY" | grep -c '^### [0-9]' || true) - TITLE="[oauth] PR #$PR_NUMBER: ${FINDING_COUNT} finding(s) — triage pending" - fi - - BODY=$(printf '**Source:** %s\n**Repo:** oauth\n**PR:** #%s\n**Model:** claude-sonnet-4-6\n**Phase:** baseline\n**Review job status:** %s\n**Date:** %s\n\n---\n\n%s\n' \ - "$PR_URL" "$PR_NUMBER" "$REVIEW_STATUS" "$(date -u +%Y-%m-%dT%H:%M:%SZ)" "$CLAUDE_BODY") - - PAYLOAD=$(jq -nc \ - --arg title "$TITLE" \ - --arg body "$BODY" \ - '{title: $title, body: $body, labels: ["repo:oauth", "verdict:pending", "phase:baseline"]}') - - HTTP=$(curl -sS -o /tmp/ai-log-resp.json -w '%{http_code}' -X POST \ - -H "Authorization: Bearer $AI_REVIEW_LOG_TOKEN" \ - -H "Accept: application/vnd.github+json" \ - -H "X-GitHub-Api-Version: 2022-11-28" \ - https://api.github.com/repos/HarperFast/ai-review-log/issues \ - -d "$PAYLOAD") - - if [ "$HTTP" -ge 200 ] && [ "$HTTP" -lt 300 ]; then - ISSUE_URL=$(jq -r '.html_url' /tmp/ai-log-resp.json) - echo "Logged review to $ISSUE_URL" - else - echo "::warning::ai-review-log POST failed (HTTP $HTTP):" - cat /tmp/ai-log-resp.json - fi + id-token: write + with: + # Same SHA as the `uses:` ref above. The reusable uses this to + # check out HarperFast/ai-review-prompts (layer files + bash + # scripts) at the same ref as the workflow logic itself — keeps + # the upgrade motion atomic. + # + # The duplication is unavoidable: reusable workflows can't + # introspect their own ref (`github.workflow_ref` resolves to the + # CALLER's ref in `workflow_call` context), and `uses: …@` + # is parsed literally so we can't interpolate a variable. + ai-review-prompts-ref: 533f841137114315b0b4d02dabf367e376d9922e + review-layers: | + universal + harper/common + harper/v5 + repo-type/plugin + repo-specific-checks: | + ## Repo-specific checks (OAuth plugin) + + On top of the layered scope above, these are OAuth-only + semantics not covered by the shared layers: + + - CSRF state tokens present on every OAuth flow; 10-minute + expiry is enforced; state is single-use + - Redirect URI validation on callback endpoints + - Provider-of-record enforcement (cross-provider CSRF + protection should redirect with error, not 403) + - Session field preservation across token refresh + (`provider`, `providerConfigId`, `providerType`) + - Path length validation (≤ 2048 chars) where user input + can reach a path + secrets: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + AI_REVIEW_LOG_TOKEN: ${{ secrets.AI_REVIEW_LOG_TOKEN }} + HARPERFAST_AI_CLIENT_ID: ${{ secrets.HARPERFAST_AI_CLIENT_ID }} + HARPERFAST_AI_APP_PRIVATE_KEY: ${{ secrets.HARPERFAST_AI_APP_PRIVATE_KEY }} diff --git a/.github/workflows/gemini-review.yml b/.github/workflows/gemini-review.yml new file mode 100644 index 0000000..0f0d67f --- /dev/null +++ b/.github/workflows/gemini-review.yml @@ -0,0 +1,84 @@ +name: Gemini PR Review + +# Thin caller of the Gemini reusable in HarperFast/ai-review-prompts. +# Runs in parallel with claude-review.yml so the two reviewers can be +# compared on the same PRs. +# +# Layer inputs and `repo-specific-checks:` MIRROR claude-review.yml +# in this repo. Output comparability between the two providers +# depends on them seeing the same review scope — keep them in sync +# when bumping the pin or editing the checks block. +# +# Pre-requisites: +# - HARPERFAST_AI_CLIENT_ID (org-level App Client ID) +# - HARPERFAST_AI_APP_PRIVATE_KEY (org-level App private key) +# - GEMINI_API_KEY (per-repo; optional — missing +# key cleanly skips the review +# with a workflow notice) +# - AI_REVIEW_LOG_TOKEN (optional — threads each run +# into a per-(PR, provider) issue +# in HarperFast/ai-review-log +# with `provider:gemini` label) + +on: + pull_request: + types: [opened, synchronize, reopened] + +concurrency: + # Different group key from claude-review so the two providers can + # run in parallel on the same PR. cancel-in-progress is per-group, + # so a synchronize push cancels the in-flight Gemini run without + # touching the Claude run (and vice versa). + group: gemini-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + review: + uses: HarperFast/ai-review-prompts/.github/workflows/_gemini-review.yml@533f841137114315b0b4d02dabf367e376d9922e # main 2026-05-21 (post #42 — continuity preamble now mandatory when prior findings exist so Gemini updates narrate the round, plus log-script count-line fix) + # Caller-side permissions, scoped at the calling-job level (NOT + # workflow-level — that placement caps the reusable's per-job + # grants below what they need and breaks the workflow at startup; + # see ai-review-prompts#39/#40 for the incident). Union of what + # the reusable's `authorize` (`contents: read`) and `review` + # (`contents: read + pull-requests: write + id-token: write`) + # jobs declare. GitHub's rule: caller's GITHUB_TOKEN permissions + # can only be DOWNGRADED (not elevated) by the called workflow, + # so the caller must grant at least the union the reusable needs. + # Mirrors claude-review.yml in this repo — surfaced as a finding + # by Gemini's own review on PR #88. + # Reference: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations + permissions: + contents: read + pull-requests: write + id-token: write + with: + # Same SHA as the `uses:` ref above. See claude-review.yml + # in this repo for why the duplication is unavoidable + # (reusable workflows can't introspect their own ref in + # workflow_call context). + ai-review-prompts-ref: 533f841137114315b0b4d02dabf367e376d9922e + review-layers: | + universal + harper/common + harper/v5 + repo-type/plugin + repo-specific-checks: | + ## Repo-specific checks (OAuth plugin) + + On top of the layered scope above, these are OAuth-only + semantics not covered by the shared layers: + + - CSRF state tokens present on every OAuth flow; 10-minute + expiry is enforced; state is single-use + - Redirect URI validation on callback endpoints + - Provider-of-record enforcement (cross-provider CSRF + protection should redirect with error, not 403) + - Session field preservation across token refresh + (`provider`, `providerConfigId`, `providerType`) + - Path length validation (≤ 2048 chars) where user input + can reach a path + secrets: + GEMINI_API_KEY: ${{ secrets.GEMINI_API_KEY }} + AI_REVIEW_LOG_TOKEN: ${{ secrets.AI_REVIEW_LOG_TOKEN }} + HARPERFAST_AI_CLIENT_ID: ${{ secrets.HARPERFAST_AI_CLIENT_ID }} + HARPERFAST_AI_APP_PRIVATE_KEY: ${{ secrets.HARPERFAST_AI_APP_PRIVATE_KEY }} diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 8ca8955..8126180 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -13,14 +13,14 @@ jobs: strategy: matrix: - node-version: [20, 22, 24] + node-version: [22, 24] steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Setup Node.js ${{ matrix.node-version }} - uses: actions/setup-node@v4 + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: node-version: ${{ matrix.node-version }} cache: 'npm' @@ -34,25 +34,19 @@ jobs: - name: Check formatting run: npm run format:check - # Coverage was failing to run on Node 20, so run it only on 22+ - - name: Run tests with coverage (Node 22+) - if: matrix.node-version >= 22 + - name: Run tests with coverage run: npm run test:coverage - - name: Run tests (Node 20) - if: matrix.node-version == 20 - run: npm run test:node-twenty - test-bun: name: Test with Bun runs-on: ubuntu-latest steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Setup Bun - uses: oven-sh/setup-bun@v2 + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 with: bun-version: latest diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f3808ab..4c416c2 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -18,7 +18,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: Setup Node.js - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: node-version: '22' registry-url: 'https://registry.npmjs.org' diff --git a/.github/workflows/validate-caller-workflows.yml b/.github/workflows/validate-caller-workflows.yml new file mode 100644 index 0000000..16e5bfa --- /dev/null +++ b/.github/workflows/validate-caller-workflows.yml @@ -0,0 +1,33 @@ +name: Validate caller workflows + +# Thin caller of HarperFast/ai-review-prompts' `_validate-caller-workflows.yml`. +# Validates `.github/workflows/claude-*.yml` caller files for: +# +# * Shadow jobs — a non-`uses:` job (or a `uses:` outside `HarperFast/`) +# alongside the legit reusable call would run with the caller's +# permissions WITHOUT going through the auth gate. Fail-closed. +# * Mutable refs in `uses:` or `with.ai-review-prompts-ref` — both +# must pin to a 40-char SHA. +# +# Runs on every PR and every push to `main` — no `paths:` filter. +# A required status check that only fires on workflow-touching PRs +# stays permanently pending on every other PR (GitHub has no +# "required if it runs" semantic). The validator's runtime is +# trivial (yq-parses a few caller files), so unconditional firing +# is the right trade-off for satisfiability. +# +# Make this `validate` job a REQUIRED status check on `main`. + +on: + pull_request: + push: + branches: [main] + +jobs: + validate: + uses: HarperFast/ai-review-prompts/.github/workflows/_validate-caller-workflows.yml@f6daed301f8a7ece74593506de38c6e80820b00b # main 2026-05-06 (post #11/#12/#14) + with: + # Same SHA as the `uses:` ref above — the reusable uses this to + # check out the validator script at the matching version. Same + # SHA-twice pattern as the other caller workflows in this repo. + ai-review-prompts-ref: f6daed301f8a7ece74593506de38c6e80820b00b From 19761ef68331bef411a3b3a7c6553113f9d303e5 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 26 May 2026 14:48:41 -0700 Subject: [PATCH 5/5] Address review feedback: backup race, cleanup unification, stricter build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acts on the agy + codex review pass on this PR. Fixes (blocker + significant): 1. **Backup-overwrite race (blocker, agy)** — relocate the boot-props backup from a hardcoded /tmp path into the per-run tempRoot. On an unclean rerun, the previous design copied the now-dirty BOOT_PROPS_PATH over the still-original backup at the hardcoded path, permanently losing the user's real ~/.harperdb config. With the backup under tempRoot (mkdtemp'd per run), prior leftover backups can't be overwritten — and an existsSync check inside backupBootProps provides a belt-and-suspenders guard if tempRoot were ever reused. 2. **Cleanup path unification (significant, codex)** — replace the process.exit() short-circuit in the signal handlers with a single synchronous `cleanup()` function that the finally block AND every signal handler call. cleanup() now: kills harperdb (with async SIGKILL fallback), restores boot props via the same code path used by normal exits, and removes tempRoot. waitForReady also subscribes to the child process's `error` event so ENOENT / EACCES route through the same cleanup. Adds SIGHUP / SIGQUIT in addition to SIGINT / SIGTERM. 3. **/tmp multi-user collision (significant, agy)** — covered by #1 (backup is now inside the run-scoped tempRoot, no shared-tmp races). 4. **node_modules cp waste (significant, agy)** — eliminate the fixtureDir → components/app recursive copy. Write config.yaml / package.json directly into tempRoot/components/app and run npm install there. Saves seconds of recursive-copy per run. 5. **Silent stale dist/ (significant, codex)** — replace `npm run build` (which is `tsc || true`) with: rm dist/, invoke the local tsc binary directly, then assert dist/index.js was emitted. A TS emission failure now stops the script instead of letting it pass against the previous build. 6. **Peer-dep registry drift (significant, codex)** — pin the fixture's harperdb peer to the exact version (4.7.19) being spawned by the script. Removes the risk that `npm install` resolves a different registry version than what the script's harperBin is. Also adopts agy's `oac-oauth-tenant` rename: the tenant provider name now contains the substring "oauth" so this script (and its v5 counterpart in PR #102) also catches an over-aggressive stripping regression in addition to the prefix-extraction case. Out of scope: the v5 layer applied to v1.x AI reviews (both reviewers flagged this; needs a branch-aware layer input in HarperFast/ai-review-prompts, not changes here). Verified locally: run produces PASS, ~/.harperdb/hdb_boot_properties.file restored to its pre-run state. Co-Authored-By: Claude Opus 4.7 (1M context) --- scripts/v4-routing-repro.mjs | 256 ++++++++++++++++++++++------------- 1 file changed, 163 insertions(+), 93 deletions(-) diff --git a/scripts/v4-routing-repro.mjs b/scripts/v4-routing-repro.mjs index 679c0c2..87efa1a 100644 --- a/scripts/v4-routing-repro.mjs +++ b/scripts/v4-routing-repro.mjs @@ -10,26 +10,30 @@ * harperdb ^4.7.28. * * Spawns harperdb v4 against a temp fixture that configures two providers — - * one literally named "oauth" (decoy) and one named "oac-tenant-acme" with - * a distinctive client_id — then fires /oauth/oac-tenant-acme/login and - * inspects the resulting redirect. + * one literally named "oauth" (decoy) and one named "oac-oauth-tenant" (the + * substring "oauth" inside this name also guards against an over-aggressive + * stripping regression, not just the prefix-extraction case). Fires + * /oauth/oac-oauth-tenant/login and inspects the resulting redirect. * * Outcomes: * - Redirect to tenant.test/authorize with the tenant client_id → parseRoute * correctly extracted the URL path segment. This is the current behavior - * on Harper v4.7.19 + v1.4.0 plugin source — Dawson's diagnosis does not - * reproduce on this stack. - * - Redirect to decoy.test/authorize → parseRoute extracted the literal - * "oauth" prefix as providerName. Bug confirmed (regression). + * on harperdb 4.7.19 + the in-tree plugin source — Dawson's diagnosis + * does not reproduce on this stack. + * - Anything else (404, decoy redirect, malformed URL) → the assertion + * fails; investigate parseRoute / the resource dispatch path. * * Side effects — harperdb's installer rewrites ~/.harperdb/hdb_boot_properties.file - * to point at the temp ROOTPATH. The script saves the original contents (if any) - * before launch and restores them in a finally block, so an existing local - * `harperdb` install is left intact across runs. If the script is killed - * uncleanly, manually restore the file from ${BOOT_BACKUP_PATH}. + * to point at the temp ROOTPATH. The script backs that file up (into the + * run's tempRoot) before launch and restores it in a single cleanup path + * shared by the normal `finally` block and the signal/exit handlers, so an + * existing local `harperdb` install is left intact across runs. If the + * script is SIGKILL'd between backup and the harperdb install actually + * touching the file, the backup is collocated with tempRoot and may be + * left orphaned in $TMPDIR until OS cleanup; check there for recovery. * * Not wired into CI. Run manually: - * npm run build && node scripts/v4-routing-repro.mjs + * node scripts/v4-routing-repro.mjs * * Set KEEP_TEMP=1 to preserve the temp dir for inspection. */ @@ -40,12 +44,19 @@ import { fileURLToPath } from 'node:url'; import { spawn, spawnSync } from 'node:child_process'; const BOOT_PROPS_PATH = join(homedir(), '.harperdb', 'hdb_boot_properties.file'); -const BOOT_BACKUP_PATH = join(tmpdir(), 'oauth-v4-repro-hdb_boot_properties.file.bak'); const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '..'); const harperBin = join(repoRoot, 'node_modules', '.bin', 'harperdb'); +const tscBin = join(repoRoot, 'node_modules', '.bin', 'tsc'); -const TENANT_CLIENT_ID = 'tenant-acme-client-id'; +// Pin the fixture's harperdb peer to the exact version we're spawning so +// `npm install` doesn't resolve a drifted version from the registry. +// Sourced from node_modules/harperdb/package.json at script-write time; +// kept in sync via the SOURCE-OF-TRUTH comment so a Renovate / manual bump +// of the repo's harperdb dep has a single human-readable place to update. +const HARPERDB_PIN = '4.7.19'; // SOURCE-OF-TRUTH: node_modules/harperdb/package.json + +const TENANT_CLIENT_ID = 'oauth-tenant-client-id'; const DECOY_CLIENT_ID = 'decoy-oauth-client-id'; const CONFIG_YAML = ` @@ -61,7 +72,7 @@ rest: true authorizationUrl: 'http://decoy.test/authorize' tokenUrl: 'http://decoy.test/token' userInfoUrl: 'http://decoy.test/userinfo' - oac-tenant-acme: + oac-oauth-tenant: provider: generic clientId: ${TENANT_CLIENT_ID} clientSecret: tenant-secret @@ -75,6 +86,10 @@ const PACKAGE_JSON = JSON.stringify( name: 'oauth-v4-routing-repro', private: true, type: 'module', + // Pin to the exact harperdb being spawned by this script so the + // fixture's npm install can't resolve a drifted registry version + // of the >=4.6.0 peer range. + devDependencies: { harperdb: HARPERDB_PIN }, }, null, 2 @@ -108,6 +123,10 @@ async function waitForReady(harperProc, timeoutMs) { cleanup(); reject(new Error(`harperdb exited prematurely with code ${code}`)); }; + const onError = (err) => { + cleanup(); + reject(new Error(`harperdb spawn error: ${err.message}`)); + }; const onTimeout = () => { cleanup(); reject(new Error(`harperdb did not become ready within ${timeoutMs}ms`)); @@ -118,64 +137,145 @@ async function waitForReady(harperProc, timeoutMs) { harperProc.stdout?.off('data', onData); harperProc.stderr?.off('data', onData); harperProc.off('exit', onExit); + harperProc.off('error', onError); }; harperProc.stdout?.on('data', onData); harperProc.stderr?.on('data', onData); harperProc.once('exit', onExit); + harperProc.once('error', onError); }); } +// Single cleanup state — populated as we go, consulted by both the +// finally block and the signal handlers so they don't redundantly try +// to restore boot props or kill harperdb. +const cleanupState = { + bootBackupPath: null, // set once tempRoot exists + bootState: 'pending', // 'pending' | 'existed' | 'absent' + tempRoot: null, + harperProc: null, + cleaned: false, +}; + function backupBootProps() { + // If a backup file already exists at this run's path it means tempRoot + // is being reused (impossible — mkdtemp is fresh each run), so existsSync + // here should always be false on a healthy invocation. The check is a + // belt-and-suspenders guard. + if (existsSync(cleanupState.bootBackupPath)) { + log(`WARNING: backup at ${cleanupState.bootBackupPath} already exists; treating as authoritative`); + cleanupState.bootState = 'existed'; + return; + } if (existsSync(BOOT_PROPS_PATH)) { - copyFileSync(BOOT_PROPS_PATH, BOOT_BACKUP_PATH); - log(`saved boot props to ${BOOT_BACKUP_PATH}`); - return 'existed'; + copyFileSync(BOOT_PROPS_PATH, cleanupState.bootBackupPath); + log(`saved boot props to ${cleanupState.bootBackupPath}`); + cleanupState.bootState = 'existed'; + return; } log('no existing boot props file to back up'); - return 'absent'; + cleanupState.bootState = 'absent'; } -function restoreBootProps(state) { +function restoreBootProps() { + const { bootBackupPath, bootState } = cleanupState; try { - if (state === 'existed') { - if (existsSync(BOOT_BACKUP_PATH)) { - copyFileSync(BOOT_BACKUP_PATH, BOOT_PROPS_PATH); - rmSync(BOOT_BACKUP_PATH, { force: true }); + if (bootState === 'existed') { + if (bootBackupPath && existsSync(bootBackupPath)) { + copyFileSync(bootBackupPath, BOOT_PROPS_PATH); log(`restored boot props from backup`); } else { - log(`WARNING: backup at ${BOOT_BACKUP_PATH} missing — leaving boot props as-is`); + log(`WARNING: backup at ${bootBackupPath} missing — leaving boot props as-is`); } - } else if (state === 'absent') { + } else if (bootState === 'absent') { + // harperdb's installer creates the file even if it wasn't there + // before. Remove it to restore the original "no boot props" state. rmSync(BOOT_PROPS_PATH, { force: true }); log(`removed boot props (none existed before)`); } } catch (err) { - log(`WARNING: failed to restore boot props: ${err.message}. Original backup at ${BOOT_BACKUP_PATH}`); + log(`WARNING: failed to restore boot props: ${err.message}. Backup may be at ${bootBackupPath}`); + } +} + +function cleanup() { + if (cleanupState.cleaned) return; + cleanupState.cleaned = true; + + const { harperProc, tempRoot } = cleanupState; + + if (harperProc && harperProc.exitCode === null) { + log('killing harperdb...'); + try { + harperProc.kill('SIGINT'); + } catch { + // Already exited between the check and the kill — fine. + } + // Best-effort follow-up SIGKILL; we can't await an async timer from + // a synchronous signal handler, so accept the brief inconsistency. + setTimeout(() => { + if (harperProc.exitCode === null) { + try { + harperProc.kill('SIGKILL'); + } catch { + // Already exited; nothing to do. + } + } + }, 1000); + } + + restoreBootProps(); + + if (tempRoot && !process.env.KEEP_TEMP) { + log(`cleaning up ${tempRoot}`); + try { + rmSync(tempRoot, { recursive: true, force: true }); + } catch (err) { + log(`WARNING: failed to remove ${tempRoot}: ${err.message}`); + } + } else if (tempRoot) { + log(`KEEP_TEMP set; leaving ${tempRoot}`); } } async function main() { - const tempRoot = mkdtempSync(join(tmpdir(), 'oauth-v4-repro-')); - const fixtureDir = join(tempRoot, 'app'); - const hdbRoot = join(tempRoot, 'hdb-root'); - log(`temp root: ${tempRoot}`); + if (!existsSync(harperBin)) { + throw new Error(`harperdb binary not found at ${harperBin}. Run "npm install" in the repo root first.`); + } + if (!existsSync(tscBin)) { + throw new Error(`tsc binary not found at ${tscBin}. Run "npm install" in the repo root first.`); + } + + cleanupState.tempRoot = mkdtempSync(join(tmpdir(), 'oauth-v4-repro-')); + cleanupState.bootBackupPath = join(cleanupState.tempRoot, 'hdb_boot_properties.file.bak'); + const componentsRoot = join(cleanupState.tempRoot, 'components'); + const componentAppDir = join(componentsRoot, 'app'); + const hdbRoot = join(cleanupState.tempRoot, 'hdb-root'); + log(`temp root: ${cleanupState.tempRoot}`); // Save the current boot props before harperdb's installer rewrites them. // Done outside the try so the backup is in place if anything below throws // before harperdb spawns. - const bootState = backupBootProps(); + backupBootProps(); - let harperProc = null; let exitCode = 1; try { - // 1. Build the plugin (so dist/ is current) - log('building plugin...'); - run('npm', ['run', 'build'], { cwd: repoRoot }); + // 1. Build the plugin with a stricter invocation than `npm run build` + // (which is `tsc || true` — silent failures can leave stale dist/). + // Clear dist/ first so a TS emission failure is visible by the + // missing dist/index.js after. + log('rebuilding plugin (strict)...'); + rmSync(join(repoRoot, 'dist'), { recursive: true, force: true }); + run(tscBin, [], { cwd: repoRoot }); + const distEntry = join(repoRoot, 'dist', 'index.js'); + if (!existsSync(distEntry)) { + throw new Error(`build did not emit ${distEntry}`); + } // 2. npm pack the local plugin into the temp dir log('packing local plugin...'); - const packResult = spawnSync('npm', ['pack', '--pack-destination', tempRoot, '--json'], { + const packResult = spawnSync('npm', ['pack', '--pack-destination', cleanupState.tempRoot, '--json'], { cwd: repoRoot, encoding: 'utf8', }); @@ -184,25 +284,22 @@ async function main() { throw new Error(`npm pack failed (exit ${packResult.status})`); } const tarballName = JSON.parse(packResult.stdout)[0].filename; - const tarballPath = join(tempRoot, tarballName); + const tarballPath = join(cleanupState.tempRoot, tarballName); - // 3. Set up the fixture - log(`writing fixture to ${fixtureDir}...`); - spawnSync('mkdir', ['-p', fixtureDir], { stdio: 'inherit' }); - writeFileSync(join(fixtureDir, 'config.yaml'), CONFIG_YAML); - writeFileSync(join(fixtureDir, 'package.json'), PACKAGE_JSON); + // 3. Set up the fixture component directly under components/app + // (avoids a wasteful cp -r of node_modules later). + log(`writing fixture to ${componentAppDir}...`); + run('mkdir', ['-p', componentAppDir]); + writeFileSync(join(componentAppDir, 'config.yaml'), CONFIG_YAML); + writeFileSync(join(componentAppDir, 'package.json'), PACKAGE_JSON); - log('installing plugin into fixture...'); - run('npm', ['install', '--no-save', '--no-audit', '--no-fund', tarballPath], { cwd: fixtureDir }); + log('installing plugin into fixture component dir...'); + run('npm', ['install', '--no-save', '--no-audit', '--no-fund', tarballPath], { cwd: componentAppDir }); // 4. Spawn harperdb with CLI-arg config — modeled on // @harperfast/integration-testing@0.3.0 (v5). Same argument names // exist in v4's bundled binary (ROOTPATH, HDB_ADMIN_*, HTTP_PORT, // OPERATIONSAPI_*, etc.). - // - // Also point Harper at the fixture directory as its componentsRoot - // so the OAuth plugin loads from there (the fixture has the plugin - // installed via npm pack above). const httpPort = 19926; const opsPort = 19925; const hostname = '127.0.0.1'; @@ -220,22 +317,17 @@ async function main() { `--LOGGING_LEVEL=debug`, `--LOGGING_STDSTREAMS=true`, `--CLUSTERING_ENABLED=false`, - `--COMPONENTSROOT=${tempRoot}/components`, + `--COMPONENTSROOT=${componentsRoot}`, ]; - // Move the fixture into a `components/` subdir so harperdb finds it - // as a component by directory name. - run('mkdir', ['-p', join(tempRoot, 'components')]); - run('cp', ['-r', fixtureDir, join(tempRoot, 'components', 'app')]); - log(`spawning: ${harperBin} ${args.join(' ')}`); - harperProc = spawn(harperBin, args); + cleanupState.harperProc = spawn(harperBin, args); log('waiting for ready...'); - await waitForReady(harperProc, 60_000); + await waitForReady(cleanupState.harperProc, 60_000); // 5. Fire the request and check the redirect. - const url = `http://${hostname}:${httpPort}/oauth/oac-tenant-acme/login`; + const url = `http://${hostname}:${httpPort}/oauth/oac-oauth-tenant/login`; log(`fetching ${url}`); const response = await fetch(url, { redirect: 'manual' }); log(`status: ${response.status}`); @@ -269,49 +361,27 @@ async function main() { log('error:', err.message); exitCode = 1; } finally { - if (harperProc && harperProc.exitCode === null) { - log('killing harperdb...'); - harperProc.kill('SIGINT'); - await new Promise((r) => setTimeout(r, 1000)); - if (harperProc.exitCode === null) harperProc.kill('SIGKILL'); - } - restoreBootProps(bootState); - if (!process.env.KEEP_TEMP) { - log(`cleaning up ${tempRoot}`); - rmSync(tempRoot, { recursive: true, force: true }); - } else { - log(`KEEP_TEMP set; leaving ${tempRoot}`); - } + cleanup(); + // Give the post-cleanup SIGKILL timer a moment to land before exiting. + await new Promise((r) => setTimeout(r, 1100)); } process.exit(exitCode); } -// Restore boot props on unclean exits too — Ctrl+C, kill, uncaught exceptions. -let signaled = false; -function emergencyRestore() { - if (signaled) return; - signaled = true; - try { - if (existsSync(BOOT_BACKUP_PATH)) { - copyFileSync(BOOT_BACKUP_PATH, BOOT_PROPS_PATH); - rmSync(BOOT_BACKUP_PATH, { force: true }); - console.log(`[v4-repro] emergency: restored boot props from ${BOOT_BACKUP_PATH}`); - } - } catch (err) { - console.error(`[v4-repro] emergency restore failed: ${err.message}`); - } +// Route signal exits through the shared cleanup path so harperdb is killed, +// tempRoot is removed, and boot props are restored before we leave. +for (const sig of ['SIGINT', 'SIGTERM', 'SIGHUP', 'SIGQUIT']) { + process.on(sig, () => { + cleanup(); + const code = sig === 'SIGINT' ? 130 : sig === 'SIGTERM' ? 143 : sig === 'SIGHUP' ? 129 : 131; + // Brief delay so the cleanup's async timer can fire if needed. + setTimeout(() => process.exit(code), 1100).unref(); + }); } -process.on('SIGINT', () => { - emergencyRestore(); - process.exit(130); -}); -process.on('SIGTERM', () => { - emergencyRestore(); - process.exit(143); -}); main().catch((err) => { console.error('[v4-repro] fatal:', err); + cleanup(); process.exit(1); });