Skip to content

feat!: Harper v5 support; fix withOAuthValidation on Resource API v2#43

Closed
heskew wants to merge 3 commits intomainfrom
fix/withOAuthValidation-resource-v2
Closed

feat!: Harper v5 support; fix withOAuthValidation on Resource API v2#43
heskew wants to merge 3 commits intomainfrom
fix/withOAuthValidation-resource-v2

Conversation

@heskew
Copy link
Copy Markdown
Member

@heskew heskew commented Apr 20, 2026

Summary

Two tightly-related changes bundled per Nathan's direction: migrate the plugin to Harper v5 (harper npm package, v5 Scope type, Resource API v2 conventions) and fix #33 (withOAuthValidation silently passing Resource API v2 requests).

BREAKING

  • Drops harperdb v4 support. Peer dep is now harper >=5.0.0.
  • The v4-compatible code line lives on v1.x branch; future v4 bug fixes should PR there.
  • Version bump to 2.0.0 is deferred until we're ready to cut the release.

Harper v5 migration

  • Peer/dev dep: harperdbharper (peer >=5.0.0, dev ^5.0.0)
  • All imports: from 'harperdb'from 'harper' (src/types.ts, src/lib/resource.ts, src/lib/handlers.ts, docs/api-reference.md)
  • Bun preload mock target: mock.module('harper', ...)
  • handleApplication entry: guard once on scope.resources / scope.server (Harper v5 types mark these optional but they're always assigned by Scope's constructor — fail loudly if the invariant is ever violated, then use locally-narrowed consts throughout instead of scattering ?. or !)
  • npm run test:coverage path pattern updated
  • CLAUDE.md + package.json keywords updated

#33 fix: withOAuthValidation on Resource API v2

  • Old code: const request = args.find((arg) => arg?.session !== undefined); — a v4/legacy pattern. Resource API v2 method signatures are receive(target, data) / get(target) etc.; the request is NOT in the args. The wrapper silently passed through without validating.
  • New code: read the request from this.getContext().
  • test/lib/withOAuthValidation.test.js — new, 7 tests covering: valid-session passthrough, requireAuth 401, requireAuth: false passthrough, custom onValidationError, stale-provider clearing, non-HTTP method passthrough, no-context fallthrough.

Prereqs already done

  • v1.x branch cut from pre-migration main (for future v4 bug fixes)
  • Label taxonomy pre-created on ai-review-log
  • AI_REVIEW_LOG_TOKEN secret configured

Test plan

  • bun test — 429 pass, 2 skip, 0 fail (22 files)
  • npm run lint passes
  • npm run format:check passes (.claude/settings.local.json warn is gitignored local-only)
  • CI — Node matrix + Bun
  • First Claude review exercising the full v2 pipeline (review + log write)

Closes #33

heskew and others added 2 commits April 20, 2026 07:06
BREAKING: drops support for harperdb v4. Use the v1.x branch for
v4-compatible releases (v1.x line).

Harper v5 migration:
- Rename peer/dev dep `harperdb` → `harper` (`>=5.0.0` / `^5.0.0`)
- Update all imports `from 'harperdb'` → `from 'harper'` (src/types.ts,
  src/lib/resource.ts, src/lib/handlers.ts, docs/api-reference.md)
- Update Bun preload mock target (`mock.module('harper', ...)`)
- Guard on `scope.resources` / `scope.server` once at
  `handleApplication` entry — the Harper v5 `Scope` type marks these
  optional but they're assigned in the constructor; fail loudly if
  the invariant is ever violated, then use locally-narrowed consts
  throughout (no scattered `?.` or `!`)
- Update `npm run test:coverage` path pattern accordingly
- Update CLAUDE.md + package.json keyword ordering

withOAuthValidation fix (closes #33):
- Old code found the request via `args.find((arg) => arg?.session)`,
  which is a v4/legacy pattern. Resource API v2 methods receive
  `(target, data)` — no request in args. The wrapper silently passed
  through without validating.
- Drop the args-based lookup; read the request from `this.getContext()`.
- Add test/lib/withOAuthValidation.test.js with 7 tests covering:
  valid-session passthrough, requireAuth 401, requireAuth-false
  passthrough, custom onValidationError, stale-provider clearing,
  non-HTTP method passthrough, and no-context fallthrough.

Version bump to 2.0.0 is deferred until we're ready to cut the release.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI `npm ci` failed because the lockfile referenced bufferutil@4.1.0,
utf-8-validate@5.0.10, and node-gyp-build@4.8.4 transitively but
lacked entries for them. Happens when `npm install` partially resolves
optional peer deps. Clean regen resolves.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Apr 20, 2026

1. Missing tests for validation failure paths — the core cases the fix was supposed to address

File: test/lib/withOAuthValidation.test.js
What: The fix changes withOAuthValidation from silently passing through (old args.find pattern) to actually calling validateAndRefreshSession via this.getContext(). The new test file only covers the happy path (valid session, lines 74–90) and the no-session case (lines 92–131). The code paths that were broken before and are now supposed to work — specifically (a) provider found but token expired + requireAuth: true (lines 152–168 in withOAuthValidation.ts) and (b) session referencing an unconfigured provider + requireAuth: true (lines 131–144) — have no test.
Why it matters: Every test that exercises a valid OAuth session returns { valid: true } from validateAndRefreshSession because the mock sessions carry no expiresAt/refreshThreshold. The failure branch (!validation.valid && requireAuth → 401) is entirely dead in the test suite. There is no evidence the fix works for the scenarios it was written to fix: a v2 resource where the token has expired should now reject with 401, but this is unverified.
Suggested fix: Add two tests — one where the session has expiresAt set to a past timestamp and no refreshToken with requireAuth: true (should return 401), and one where the session's provider field names a provider absent from the registry with requireAuth: true (should return 401). The existing "clears stale session data" test already covers the requireAuth: false side of the second case.

assert.equal(calls.length, 1);
});
});
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Two test cases are needed here to cover the failure paths introduced by the fix:

Suggested change
});
describe('fallthrough: no context, no args with session', () => {
it('passes through when the resource has no getContext and no request is in args', async () => {
// Simulates a method called without v2 context and without a legacy request arg
const calls = [];
const resource = {
async get(target, data) {
calls.push({ target, data });
return { status: 200 };
},
};
const wrapped = withOAuthValidation(resource, { providers: mockProviders, logger: mockLogger });
const result = await wrapped.get({ path: '/noop' }, 'irrelevant');
assert.equal(result.status, 200);
assert.equal(calls.length, 1);
});
});
describe('validation failure paths (the scenarios the fix was written to address)', () => {
it('returns 401 when requireAuth is true and the session token is expired with no refresh token', async () => {
const expiredSession = makeSession({
oauth: {
provider: 'github',
accessToken: 'old-token',
refreshToken: undefined,
expiresAt: Date.now() - 1000, // already expired
},
});
const context = { session: expiredSession };
const calls = [];
const resource = makeV2Resource((method) => {
calls.push({ method });
return { status: 200 };
}, context);
const wrapped = withOAuthValidation(resource, {
providers: mockProviders,
logger: mockLogger,
requireAuth: true,
});
const result = await wrapped.get({ path: '/protected' });
assert.equal(result.status, 401);
assert.equal(calls.length, 0, 'underlying method must not be called for expired session');
});
it('returns 401 when requireAuth is true and the session provider is not in the registry', async () => {
const context = {
session: makeSession({ oauth: { provider: 'ghost-provider', accessToken: 'stale' } }),
};
const calls = [];
const resource = makeV2Resource((method) => {
calls.push({ method });
return { status: 200 };
}, context);
const wrapped = withOAuthValidation(resource, {
providers: mockProviders, // only has 'github'
logger: mockLogger,
requireAuth: true,
});
const result = await wrapped.get({ path: '/protected' });
assert.equal(result.status, 401);
assert.equal(calls.length, 0, 'underlying method must not be called when provider is unknown');
});
});
});

@heskew heskew mentioned this pull request Apr 20, 2026
3 tasks
@heskew
Copy link
Copy Markdown
Member Author

heskew commented Apr 20, 2026

Closing in favor of a split: (A) Harper v5 migration, (B) #33 withOAuthValidation fix layered on top of A. Splitting keeps each PR within the reviewer's turn budget and preserves the option to backport the fix to the v1.x branch independently.

Branch fix/withOAuthValidation-resource-v2 stays as a historical reference — commits are not deleted. New work will open fresh branches from main after PR #45 (max-turns bump) lands.

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.

withOAuthValidation doesn't work with Resource API v2 classes

1 participant