Skip to content

feat(adapters): adapter middleware framework (base) - #1518

Closed
Glorf wants to merge 2 commits into
NVIDIA-NeMo:feat/ecs-fargate-sandboxfrom
Glorf:feat/adapter-base
Closed

feat(adapters): adapter middleware framework (base)#1518
Glorf wants to merge 2 commits into
NVIDIA-NeMo:feat/ecs-fargate-sandboxfrom
Glorf:feat/adapter-base

Conversation

@Glorf

@Glorf Glorf commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Introduces an interceptor-based middleware framework that runs at every in-tree server's request/response boundary. PR 1 of 4 — base framework only. Follow-on PRs add observability / caching / request-rewriting interceptor families.

Framework

nemo_gym/adapters/
pipeline.py AdapterPipeline + stage-order validation
(REQUEST → REQUEST_TO_RESPONSE → RESPONSE)
middleware.py install_middleware(app, specs) — FastAPI middleware
that wraps call_next; body replay, multi-Set-Cookie
preservation, /s//... session-prefix routing,
GracefulError → 429
proxy.py start_adapter_proxy(upstream_url, adapters) —
localhost uvicorn host mode for external-inference
agents whose SDK respects *_BASE_URL env vars
registry.py short-name → Interceptor class + runtime register()
types.py AdapterRequest/Response, three Interceptor ABCs,
Stage enum, GracefulError, ContextVar-backed
per-request context, InterceptorSpec and
AdapterProxyConfig pydantic models
interceptors/
endpoint.py Drives upstream HTTP call (required by proxy mode,
forbidden inside install_middleware)
request_logging.py
Canonical "did the chain fire?" probe (logs body
keys + response status/latency)

Server wire-up

adapters: list[dict] | None = None on three base configs:
BaseResponsesAPIModelConfig
BaseResponsesAPIAgentConfig
BaseResourcesServerConfig

install_middleware(app, self.config.adapters) at the tail of each
base's setup_webserver. Every in-tree server inheriting from
SimpleResponsesAPI{Model,Agent} or SimpleResourcesServer picks up
the adapters knob automatically.

adapter_proxy: AdapterProxyConfig | None = None on
BaseResponsesAPIAgentConfig. When set, SimpleResponsesAPIAgent.
setup_webserver starts a localhost uvicorn proxy in a daemon thread,
stores the ProxyHandle on self._proxy_handle, registers atexit cleanup.

Per-server override fixes

harbor_agent and mini_swe_agent override setup_webserver without
calling super(). Inline install_middleware(app, self.config.adapters)
calls added to both so the adapters field still applies.

claude_code_agent integration

Reads self._proxy_handle.url when adapter_proxy is set; threads the
proxy URL into the claude CLI subprocess via ANTHROPIC_BASE_URL +
ANTHROPIC_AUTH_TOKEN. Preserves the full model-name prefix in proxy
mode (no .split("/")[-1] stripping).

Safety

  • Stage ordering validated at startup
  • Unknown interceptor name raises at config-validation time
  • install_middleware rejects endpoint in chain (host already forwards)
  • start_adapter_proxy rejects user-supplied endpoint AND refuses host="0.0.0.0" unless unsafe_allow_remote=True (otherwise leaks the upstream API key to any caller on the network)
  • best_effort=True interceptors swallow exceptions; strict ones propagate

Tests

42 tests, framework-level coverage. Follow-on PRs add per-interceptor
test suites.

Docs

fern/versions/latest/pages/model-server/adapters.mdx new
fern/versions/latest/pages/model-server/index.mdx link added

@copy-pr-bot

copy-pr-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Glorf
Glorf force-pushed the feat/adapter-base branch from 28ca7e6 to 18530a2 Compare June 8, 2026 10:46
Comment on lines +26 to +33
This PR ships the **framework** (`AdapterPipeline`, `InterceptorRegistry`, `install_middleware`, `start_adapter_proxy`) plus two built-in interceptors:

| Name | Stage | Purpose |
|------|-------|---------|
| `logging` | request + response | Log request body keys and response status/latency. Canonical "did the chain fire?" probe. |
| `endpoint` | request → response | Drive the upstream HTTP call directly. **Only used by `start_adapter_proxy`** (standalone host mode); forbidden inside `install_middleware`. |

Additional interceptor families (observability, caching, request rewriting) ship in follow-on PRs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldnt have this in the docs

Comment on lines +186 to +188
if m:
ctx.extra["session_id"] = m.group(1)
path = m.group(2) or "/"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

session_id plumbed through here in extra

Comment on lines +103 to +104
ctx = InterceptorContext(request_id=request.ctx.request_id)
set_context(ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this drops the session id from the request? https://github.com/NVIDIA-NeMo/Gym/pull/1518/changes#r3380938233

please add a test for this

Comment on lines +80 to +83
async for chunk in body_iter:
if isinstance(chunk, str):
chunk = chunk.encode("utf-8")
chunks.append(chunk)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this means we can no longer stream responses? we need the complete body now.

my understanding, please correct me if I am wrong:

for a given JSON response, it's

  • produced as bytes by the route:
  • buffered into an in-memory copy
  • parsed to a dict
  • maybe mutated by the interceptor
  • re-serialized to bytes
  • sent

do you have a perf benchmark with and without the adapters added to measure the overhead?

my primary concern is with losing streaming support. with all gym servers currently running on the same node, this change also exacerbates CPU resource usage with the extra ser/de + allocations

Comment on lines +171 to +172
if request.method != "POST":
return await call_next(request)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why does middleware wrap everything wheras the localhost proxy has a specific allow-list of routes to wrap?

wont this mean someone authoring an interceptor has to be aware of how it's deployed (middleware vs proxy?)

def _build_app(pipeline: AdapterPipeline, upstream_url: str, request_timeout: float) -> FastAPI:
@asynccontextmanager
async def _lifespan(app_: FastAPI):
app_.state.session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=request_timeout))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

need to add a comment to document this + connection settings, since this it outside gym's global client session

return _current_context.get()
except LookupError:
ctx = InterceptorContext()
_current_context.set(ctx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this mutates the current context, but is used in the AdapterRequest + AdapterResponse context constructors via the default factory. the constructor updating global state is surprising

Comment on lines +101 to +106
if host not in ("127.0.0.1", "localhost") and not unsafe_allow_remote:
raise ValueError(
f"start_adapter_proxy: refusing host={host!r} — the proxy forwards the "
"client's Authorization header upstream, so binding to a non-localhost "
"interface leaks credentials. Pass unsafe_allow_remote=True to override."
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how will this work if we install an agent in a sandbox? this works now for claude code because its run through a subprocess, but after this won't be shared right? #1377

we also have a lifecycle mismatch there, since the proxy is starte donce per agent server process spinup, whereas the sandbox is launched once per rollout.

would it be better to point the base url to the model server url and use the middelware approach fully? that way the model server is the one place where we have this integration, independent of the whole agent running in sandbox vs gym-native agent spinning up sandboxes + calling the model server endpoint?

in this case we'd need the sandbox to be able to reach the model server URL right?

cc @hemildesai

Glorf added 2 commits June 22, 2026 14:37
Introduces an interceptor-based middleware framework that runs at every
in-tree server's request/response boundary. PR 1 of 4 — base framework
only. Follow-on PRs add observability / caching / request-rewriting
interceptor families.

Framework

  nemo_gym/adapters/
    pipeline.py      AdapterPipeline + stage-order validation
                     (REQUEST → REQUEST_TO_RESPONSE → RESPONSE)
    middleware.py    install_middleware(app, specs) — FastAPI middleware
                     that wraps call_next; body replay, multi-Set-Cookie
                     preservation, /s/<hex>/... session-prefix routing,
                     GracefulError → 429
    proxy.py         start_adapter_proxy(upstream_url, adapters) —
                     localhost uvicorn host mode for external-inference
                     agents whose SDK respects *_BASE_URL env vars
    registry.py      short-name → Interceptor class + runtime register()
    types.py         AdapterRequest/Response, three Interceptor ABCs,
                     Stage enum, GracefulError, ContextVar-backed
                     per-request context, InterceptorSpec and
                     AdapterProxyConfig pydantic models
    interceptors/
      endpoint.py    Drives upstream HTTP call (required by proxy mode,
                     forbidden inside install_middleware)
      request_logging.py
                     Canonical "did the chain fire?" probe (logs body
                     keys + response status/latency)

Server wire-up

  adapters: list[dict] | None = None  on three base configs:
    BaseResponsesAPIModelConfig
    BaseResponsesAPIAgentConfig
    BaseResourcesServerConfig

  install_middleware(app, self.config.adapters)  at the tail of each
  base's setup_webserver. Every in-tree server inheriting from
  SimpleResponsesAPI{Model,Agent} or SimpleResourcesServer picks up
  the adapters knob automatically.

  adapter_proxy: AdapterProxyConfig | None = None  on
  BaseResponsesAPIAgentConfig. When set, SimpleResponsesAPIAgent.
  setup_webserver starts a localhost uvicorn proxy in a daemon thread,
  stores the ProxyHandle on self._proxy_handle, registers atexit cleanup.

Per-server override fixes

  harbor_agent and mini_swe_agent override setup_webserver without
  calling super(). Inline install_middleware(app, self.config.adapters)
  calls added to both so the adapters field still applies.

claude_code_agent integration

  Reads self._proxy_handle.url when adapter_proxy is set; threads the
  proxy URL into the claude CLI subprocess via ANTHROPIC_BASE_URL +
  ANTHROPIC_AUTH_TOKEN. Preserves the full model-name prefix in proxy
  mode (no .split("/")[-1] stripping).

Safety

  - Stage ordering validated at startup
  - Unknown interceptor name raises at config-validation time
  - install_middleware rejects `endpoint` in chain (host already forwards)
  - start_adapter_proxy rejects user-supplied `endpoint` AND refuses
    host="0.0.0.0" unless unsafe_allow_remote=True (otherwise leaks
    the upstream API key to any caller on the network)
  - best_effort=True interceptors swallow exceptions; strict ones
    propagate

Tests

  42 tests, framework-level coverage. Follow-on PRs add per-interceptor
  test suites.

Docs

  fern/versions/latest/pages/model-server/adapters.mdx       new
  fern/versions/latest/pages/model-server/index.mdx          link added

Signed-off-by: Michal Bien <mbien@nvidia.com>
Raise core coverage past the 96% gate (93.29% -> 96%) with real unit tests
for the previously-untested adapter paths:

- endpoint interceptor: upstream-URL suffix stripping, None-content
  normalization, success path (body/header merge, api-key injection,
  hop-by-hop stripping), retry-on-status, timeout -> 504, ClientError
  raise/retry, and close().
- adapter internals: lazy context creation in a fresh contextvars.Context,
  AdapterResponse.ok, request_logging._trunc_preview branches, middleware
  request/response conversion (streaming/plain/non-json bodies, bytes body)
  and the endpoint guard, proxy host/endpoint guards + _bind_port/
  _wait_for_health, and registry/pipeline error + best-effort paths.

All upstream HTTP/SSH is mocked; tests are offline and deterministic.

Signed-off-by: Michal Bien <mbien@nvidia.com>
@Glorf
Glorf force-pushed the feat/adapter-base branch from 18530a2 to 35a1b91 Compare June 22, 2026 12:52
@Glorf
Glorf requested a review from a team as a code owner June 22, 2026 12:52
@github-actions

Copy link
Copy Markdown
Contributor

🌿 Preview your docs: https://nvidia-preview-feat-adapter-base.docs.buildwithfern.com/nemo/gym

Here are the markdown pages you've updated:

@Glorf
Glorf changed the base branch from main to feat/ecs-fargate-sandbox June 22, 2026 13:00
@Glorf

Glorf commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by an upstream-head PR so it can join the GitHub stack (fork-head PRs can't be stacked).

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.

2 participants