feat(models): OpenAI-compatible /v1/* gateway as built-in Resources (#631, Phase 4 of #510)#1616
Draft
heskew wants to merge 15 commits into
Draft
feat(models): OpenAI-compatible /v1/* gateway as built-in Resources (#631, Phase 4 of #510)#1616heskew wants to merge 15 commits into
heskew wants to merge 15 commits into
Conversation
Registers three REST resources on the REST port when `modelsGateway: {}`
is present in config:
POST /v1/chat/completions — streaming and non-streaming chat
POST /v1/embeddings — embedding endpoint
GET /v1/models — enumerate registered backends
Key design points:
- OpenAI SDK sends `Accept: application/json` for ALL requests (incl.
streaming). Harper's REST layer only dispatches `Accept: text/event-stream`
as CONNECT; everything else is dispatched as the HTTP method. So
`stream: true` chat requests land in `post()`, not `connect()`.
The resource returns `{ body: Readable }` which REST.ts bypasses
serialisation on (REST.ts:165-193).
- Fixes a latent bug in `transformIterable` (contentTypes.ts): the
transform function was called on the terminal `{ done: true }` step of
async generators, crashing when `serialize()` received `undefined`.
Guard added: skip transform on any `done: true` step.
- Pure shape-mapper layer in `v1/translation.ts`; all functions are
side-effect-free and fully unit-tested without a running server.
- OpenAI error envelope (`{ error: { message, type, code, param } }`)
built in `v1/errors.ts`; resources catch errors themselves to avoid
REST.ts serialising them as RFC 9457 Problem Details.
- `tool_choice: 'auto'` maps to `toolMode: 'return'`; full in-process
orchestration is #612 (out of scope for this PR).
- `openai` added as devDependency (^6.45.0) for future integration tests.
- `listBackends(kind)` added to backendRegistry.ts for GET /v1/models.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…#631) Adds an integration test that starts a real Harper instance with: - `modelsGateway: {}` in config - A deterministic echo backend registered via the registerFromModule path (CJS fixture at integrationTests/server/fixtures/v1-gateway-test-backend.cjs) Tests all three endpoints: - GET /v1/models — model list shape - POST /v1/embeddings — single and batched input; 400 error shape - POST /v1/chat/completions — non-streaming shape; 400 error shape; streaming via the real OpenAI Node.js SDK (validates full SSE framing) The streaming test specifically exercises the SSE serving-path: the OpenAI SDK sends Accept: application/json for all requests, so stream: true lands in post() not connect(). The resource returns { body: Readable } which REST.ts bypasses serialisation on, and the SDK successfully parses the [DONE]-terminated SSE stream. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an OpenAI-compatible '/v1/*' REST gateway for Harper, adding endpoints for listing models, generating embeddings, and handling both streaming and non-streaming chat completions. It also resolves a critical bug in 'transformIterable' that caused crashes during the terminal step of streaming responses. Feedback from the review highlights opportunities to propagate the client's 'AbortSignal' to save server resources on disconnects, strengthen request body validation (such as checking for arrays in the embeddings endpoint and validating message structures), and defensively handle tool call arguments to prevent double-serialization.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. The non-blocking 403 |
npm ci in CI was failing with "Missing: openai@6.45.0 from lock file" because package-lock.json was not updated when openai was added to devDependencies. Ran `npm install openai --package-lock-only`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use `modelsGateway: { enabled: true }` in the integration test; an empty
object `{}` is silently dropped by `flattenObject()` in
`harperConfigEnvVars.ts` (no leaf paths → nothing set via HARPER_SET_CONFIG)
so `componentLoader` never sees the key and skips loading the gateway
- Add `|| Array.isArray(body)` guard to `V1Embeddings.post` (Gemini: arrays
pass `typeof x !== 'object'` unchanged since `typeof [] === 'object'`)
- Guard against pre-serialised string arguments in `toOAIToolCalls` (Gemini:
passing a JSON string through `JSON.stringify` would double-encode it)
- Rename inner `body` → `readable` in `V1ChatCompletions.post` streaming path
(Claude bot: shadowed the outer `body` parameter at line 28)
- AbortSignal thread left as informational: signal is already propagated via
`resolveCallContext` / `contextStorage.getStore()?.signal` in `Models.ts`
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Jul 6, 2026
server/REST.ts builds `request.data` via the streaming JSON deserializer and hands it to resource methods unawaited, so the `body` argument V1ChatCompletions.post/V1Embeddings.post received was a Promise, not the parsed object. Every real JSON POST to the gateway was reading undefined fields off a Promise and returning 400 — the integration test's 200 assertions were masked by the (separate) 404 gating bug and had never actually exercised this path. Await the body before reading any field, including `stream`. Adjudicated cross-model review finding on #1616. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Static method overrides on V1Models, V1ChatCompletions, and V1Embeddings bypass Resource's transactional() wrapper and therefore the default allowRead/allowCreate super_user gate. Add authorizeV1Request() in errors.ts that returns a well-formed OpenAI error envelope (401 for anonymous, 403 for non-super_user, null to proceed). Call it at the top of each handler before touching the body. Unit tests cover: 401 anon, 403 non-super_user, 200 super_user, and that auth is checked before body validation (prevents body deserialization on unauthorized requests). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…bled flag
Previously the gateway only loaded if the user added a `modelsGateway:` key
to harperdb-config.yaml (presence-gating). On CI and fast-startup environments
the HARPER_SET_CONFIG env var raced component loading, causing intermittent 404s.
Add `modelsGateway: { enabled: false }` to defaultConfig.yaml so the key is
always present in the resolved config and componentLoader always sees it. The
`handleApplication` entry point now checks `scope.options.get(['enabled'])` and
returns immediately when false (same pattern as the agent component).
Opt in with `modelsGateway: { enabled: true }`. The integration test already
passes this explicitly; its new header comment notes the dependency on #1618
(env-config ordering) for reliable CI behaviour.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CI's Format Check runs the lockfile prettier (3.9.3); the files were formatted with 3.8.3 which disagrees on these three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…— REVERT BEFORE MERGE Logs per-thread: componentLoader's resolved modelsGateway config + env-var presence, handleApplication's enabled value, and (test-side) the child's harper-config.yaml block + get_configuration.modelsGateway. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Internal error strings (backend stack details, paths) don't belong in the wire response. 4xx messages are client-actionable and pass through unchanged. Adjudicated cross-model review suggestion on #1616. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) — REVERT BEFORE MERGE Probes the install/config path this time: createConfigFile post-env-apply + post-validate, updateConfigValue entry + pre-write, per-thread loadRootComponents config view, plus the round-1 loader/handler probes. Test-side dump intentionally NOT restored (round 1's before() awaits delayed the first request and masked the race — that run went green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
401 = bad/missing credentials (authentication_error); 403 = valid credentials lacking permission (permission_error) — matching authorizeV1Request's own envelope. Addresses claude-bot review feedback on #1616. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
heskew
added a commit
that referenced
this pull request
Jul 8, 2026
…ush (#1618) OptionsWatcher re-parsed the on-disk root config, while componentLoader decides from the resolved in-memory config — which deterministically includes runtime env config (HARPER_SET_CONFIG et al.). At boot the env-applied values reach the file only after the first thread's runtime re-apply flushes them, so a component's boot-time scope.options reads (e.g. an enabled gate in handleApplication) could observe pre-env values the loader never saw. Proven via instrumented CI runs on #1616: componentLoader logged enabled:true on every thread of a failing run while the routes 404'd. OptionsWatcher now composes the env layers (composeConfigFromEnv, side-effect free, same precedence as the runtime pipeline) over every root-config read, including the install window where the file does not exist yet. Application config.yaml scopes are untouched, as is behavior when no config env vars are set. flattenObject additionally warns once per path when an env-var config value is an empty object — it flattens to nothing by design (load-bearing for restore-on-removal semantics) but the silent drop has confused users. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
heskew
added a commit
that referenced
this pull request
Jul 8, 2026
…ush (#1618) OptionsWatcher re-parsed the on-disk root config, while componentLoader decides from the resolved in-memory config — which deterministically includes runtime env config (HARPER_SET_CONFIG et al.). At boot the env-applied values reach the file only after the first thread's runtime re-apply flushes them, so a component's boot-time scope.options reads (e.g. an enabled gate in handleApplication) could observe pre-env values the loader never saw. Proven via instrumented CI runs on #1616: componentLoader logged enabled:true on every thread of a failing run while the routes 404'd. OptionsWatcher now composes the env layers (composeConfigFromEnv, side-effect free, same precedence as the runtime pipeline) over every root-config read, including the install window where the file does not exist yet. Application config.yaml scopes are untouched, as is behavior when no config env vars are set. flattenObject additionally warns once per path when an env-var config value is an empty object — it flattens to nothing by design (load-bearing for restore-on-removal semantics) but the silent drop has confused users. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 participants
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
OpenAI-compatible
/v1/*gateway as built-in core Resources (Phase 4 of #510):POST /v1/embeddings,POST /v1/chat/completions(streaming + non-streaming), andGET /v1/models— thin protocol-translation wrappers overscope.models. Unmodified OpenAI SDK / LangChain.js clients pointbaseURLat Harper's REST port and work.resources/models/v1/(new module):translation.ts(pure OpenAI↔internal mappers),errors.ts(OpenAI{error:{message,type,code,param}}on all error paths; 5xx messages are generic with the real error logged),embeddings.ts,chatCompletions.ts,models.ts,index.ts.modelsGateway: { enabled: false }ships in defaultConfig.yaml; enable withmodelsGateway: { enabled: true }. Explicit-flag on an always-present key, not presence-gating (deliberate: presence-gating fails silently — see HARPER_SET_CONFIG applied after component loading (thread-timing race) and not persisted at install — env-var-configured components silently fail to register #1618).authentication_error; authorization mirrors the default Resource gate (super_user).openaiStream()(Add openaiStream() formatter helper for OpenAI-compatible SSE streaming #514) through the fixedtransformIterable(see below);AbortSignalpropagates via ALS (resolveCallContextcapturesrequest.signalsynchronously — verified in adjudication, no explicit wiring needed).tool_choiceships the'return'path only ('auto' orchestration = Add agent-loop orchestration /toolMode: 'auto'toscope.models#612). Model-id routing is safe upstream: @embed / models.embed no longer forwards the logical model name as the provider wire model id #1596'stoBackendOptsstrips the logical routing key before backend dispatch, so provider calls always use the backend's configured wire model.openai@^6.45.0devDep for the end-to-end SDK test.transformIterablefix (core serving path)server/serverHelpers/contentTypes.tstransformIterable()calledtransform(step.value)on the terminal{done: true, value: undefined}step, so thetext/event-streamserializer threw at the end of any finite async-iterable response body (latent until now: existing SSE is infinite subscriptions closed via.return()). Minimal guard, both sync and async paths; covered by a real-HTTP + real-eventsourceunit test through the fullserializeStreampath.Known CI caveat — integration test flake until #1618 lands
integrationTests/server/v1-gateway.test.tspasses or fails as a unit, run-to-run, on identical code (~50% on Linux CI; macOS reliably passes). Root cause was pinned via instrumented CI runs and is NOT in this PR:scope.options(OptionsWatcher) re-parses the on-disk config file, which races the env-var config flush at boot — sohandleApplication'senabledcheck can read pre-env values even though componentLoader itself sawenabled: true(probe-verified on both threads of a failing run). The fix is up as the #1618 PR (OptionsWatcher composes env layers over root-config reads); once it merges this test is deterministic. Instrumentation commits were reverted (58bb6e867/2f000ea13+ reverts in history).Resolved in review (cross-model, adjudicated)
Body-await on POST handlers (
request.datais a Promise on the REST path — the integration test's 200 assertions had been masked by the CI 404 and had never actually validated); auth enforcement (above); model-id conflation (fixed upstream by #1596, with tests); 5xx message hygiene; 3 gemini inline suggestions applied earlier. The AbortSignal PR-body claim was verified TRUE by the adjudicator (ALS capture atModels.ts:151,202).Open for review
super_user(default-Resource parity) — deliberately conservative; loosen to a dedicated permission if desired.transformIterableguard is the one change outside the new module.Docs
Companion PR: HarperFast/documentation#568 (synced to the
enabled: trueform and the 401/super_user auth behavior).Closes #631
Tracking: #510
Generated by LLMs (Claude Sonnet 5 implementation workers + Claude Fable 5 coordination/diagnosis, via Claude Code). Cross-model reviewed: Codex leg + Harper-domain adjudication (Gemini leg failed on tooling that day — no second-model perf coverage; noted per skill protocol).
🤖 Generated with Claude Code