[VDR][v0.5.0] Public API / extension contract

🟡 API-c62d9a95 — ServerClient._build_server_base_url is private-named but is de-facto public API

Severity: medium Path: nemo_gym/server_utils.py:326 Found by: api-consistency-checker Status: new in this review

The underscore prefix signals internal-only, yet 11 first-party servers and agents call self.server_client._build_server_base_url(...) to resolve a downstream model URL, and the agent base class's own resolve_model_base_url() calls it too. One agent even monkey-patches it on a mock client in tests. Third-party authors copying these patterns take a dependency on a symbol carrying no stability guarantee.

Suggested fix: Promote it to a public ServerClient.server_base_url(server_config) (keeping _build_server_base_url as a deprecated alias via the existing moved_attr_getter shim), and migrate the 11 call sites plus resolve_model_base_url.

+++ Evidence

11 call sites, e.g. resources_servers/xstest/app.py:178, responses_api_agents/codex_agent/app.py:532, claude_code_agent/app.py:317, opencode_agent/app.py:247; base_responses_api_agent.py:133 uses it too

+++

🟡 API-22cc6e45 — HTTP route versioning is inconsistent across the three server types

Severity: medium Path: nemo_gym/base_resources_server.py:135 Found by: api-consistency-checker Status: new in this review

The model server registers every route under /v1/ (/v1/chat/completions, /v1/responses, /v1/messages). The agent server mixes conventions: /v1/responses is versioned while /run and /aggregate_metrics are not. The resources server is entirely unversioned (/verify, /seed_session, /aggregate_metrics, /reverify_mode). Users integrating against the HTTP surface must memorize which endpoints carry the prefix.

Suggested fix: Pick one rule and document it — e.g. /v1/ only for OpenAI/Anthropic-compatible passthrough routes, bare paths for Gym-native control routes — and state it in the reference docs. If Gym-native routes should be versioned, add /v1/ aliases now and keep the bare paths as permanent redirects.

+++ Evidence

model: /v1/chat/completions, /v1/responses, /v1/messages ; agent: /v1/responses, /run, /aggregate_metrics ; resources: /verify, /seed_session, /aggregate_metrics, /reverify_mode

+++

🟡 API-d4c1531d — No extension point for wrapping verify(); a server resorts to mutating the route table

Severity: medium Path: resources_servers/litmus_agent/app.py:653 Found by: api-consistency-checker Status: new in this review

SimpleResourcesServer.setup_webserver() binds /verify directly to self.verify with no pre/post hook. To reap a sandbox after verification, litmus_agent filters the base-registered route out of app.router.routes and re-registers its own handler. This reaches into Starlette internals to work around a missing hook, and would silently break if the base ever registered the route differently.

Suggested fix: Either give SimpleResourcesServer an overridable verify_handler / post-verify hook (or a documented cleanup callback keyed off session teardown), or document overriding verify() itself with a super().verify(body) call as the sanctioned pattern.

+++ Evidence

app.router.routes = [r for r in app.router.routes if getattr(r, "path", None) != "/verify"] ; app.post("/verify")(self._verify_and_cleanup)

+++

🟡 API-2e033acf — REVERIFY_MODE contract declared by only 4 of 100 resources servers

Severity: medium Path: nemo_gym/base_resources_server.py:80 Found by: api-consistency-checker Status: new in this review

BaseResourcesServerConfig.REVERIFY_MODE defaults to ReverifyMode.UNKNOWN, and only 4 servers override it. _check_reverify_mode treats UNKNOWN identically to UNSUPPORTED, so 96% of environments trip the reverify guard and require --force. An opt-in class attribute that almost nobody sets is an API that exists but does not function.

Suggested fix: Either make REVERIFY_MODE a required declaration on new servers (enforced by the existing add-verified-flag-style pre-commit hook) or default stateless servers to STATELESS and require only stateful ones to opt out. Also mention it in the environment tutorials, which currently never do.

+++ Evidence

$ grep -rln REVERIFY_MODE resources_servers/*/app.py | wc -l -> 4 ; rollout_reverification.py:508: if mode in (ReverifyMode.UNSUPPORTED, ReverifyMode.UNKNOWN)

+++

🟡 API-ae85c07e — Importing the public package mutates global interpreter state

Severity: medium Path: nemo_gym/__init__.py:150 Found by: api-consistency-checker Status: new in this review

import nemo_gym replaces builtins.print with an always-flushing variant, rewrites sys.path[:] wholesale via _augment_sys_path(), and sets TOKENIZERS_PARALLELISM/HF_HOME/HF_DATASETS_CACHE in os.environ. These are process-global side effects triggered by importing a library, which is surprising for anyone embedding Gym inside a larger application and can silently change unrelated code's behavior.

Suggested fix: Move the builtins patch, sys.path rewrite, and env-var defaults behind an explicit nemo_gym.bootstrap() that the gym CLI and server entrypoints call, leaving a bare import nemo_gym side-effect-free. At minimum, document these effects at the top of the module and gate them on an env var.

+++ Evidence

$ python -c 'import sys; p=print; import nemo_gym; ...' -> print replaced: True -> print_always_flushes ; TOKENIZERS_PARALLELISM: false ; HF_HOME: True

+++

🔵 API-50e16a94 — = Body() declaration style differs between the resources base and the agent/model bases

Severity: low Path: nemo_gym/base_resources_server.py:171 Found by: api-consistency-checker Status: new in this review

Every abstract method on SimpleResponsesAPIAgent and SimpleResponsesAPIModel declares its body parameter as body: T = Body(), while SimpleResourcesServer.verify() and seed_session() declare a bare body: T. Functionally equivalent under FastAPI, but an author writing both an agent and a resources server sees two conventions for the same thing and has to guess which matters.

Suggested fix: Normalize on one form across all five base classes — = Body() is the more explicit and already the majority — and apply it to verify() and seed_session().

+++ Evidence

verify sig : (self, body: BaseVerifyRequest) -> BaseVerifyResponse ; agent run : (self, body: BaseRunRequest = Body(PydanticUndefined)) -> BaseVerifyResponse

+++

🔵 API-01832ec2 — No documented deprecation or API-stability policy

Severity: low Path: CONTRIBUTING.md:26 Found by: api-consistency-checker Status: new in this review

The mechanics of deprecation are well built — cli/_compat.py provides moved_attr_getter, cli/legacy.py prints migration notices, and config_types.py emits DeprecationWarning for legacy dataset identifiers. But nothing states which symbols are public and stable, how long a deprecated alias survives, or what versioning scheme (__version__ is 0.5.x) implies for breakage. There is no CHANGELOG at the repo root.

Suggested fix: Add a short stability policy to CONTRIBUTING.md or the reference docs: define the public surface (ideally by pointing at the __all__ from API-001), state the minimum deprecation window in releases, and note that everything else is internal. Pair it with a root CHANGELOG.md.

+++ Evidence

CONTRIBUTING.md:26 — "Features and breaking changes: Open an issue to discuss before implementing" (only stability-related line); `ls | grep -i change` -> no results

+++

🔵 API-7373e000 — Server and request-model naming drifts from the dominant convention

Severity: low Path: resources_servers/simpleqa/app.py:231 Found by: api-consistency-checker Status: new in this review

Roughly 90 servers name their class <Name>ResourcesServer, but about 10 use the shorter <Name>Server (SimpleQAServer, AbstentionServer, OmniscienceServer, MultiChallengeServer, InverseIFServer, LongmtEvalServer, ArenaJudgeServer, ...) and two use <Name>Env (BlackjackEnv, ExampleMultiTurnEnv). Several request models also diverge from their directory name, so grepping by env name does not find the type.

Suggested fix: Add a lightweight pre-commit check (alongside the existing add-verified-flag hook) asserting that a resources server's class ends in ResourcesServer and that its request/response models are prefixed consistently with the directory name. Apply to new servers only to avoid a mass rename.

+++ Evidence

class names: SimpleQAServer, AbstentionServer, OmniscienceServer, MultiChallengeServer, BlackjackEnv, ExampleMultiTurnEnv ; dir->type drift: code_gen->CompCodingVerifyRequest, verifif->TuringVIFVerifyRequest

+++

🔵 API-cda6750e — Two servers' verify responses are only structurally, not nominally, BaseVerifyResponse

Severity: low Path: resources_servers/tavily_search/app.py:120 Found by: api-consistency-checker Status: new in this review

TavilySearchVerifyResponse(TavilySearchVerifyRequest, JudgeEvaluation) picks up reward: float from JudgeEvaluation rather than from BaseVerifyResponse, so it is wire-compatible but fails isinstance(x, BaseVerifyResponse). The same pattern appears in browsecomp_advanced_harness. Any future tooling that narrows on the declared base type will silently skip these two servers.

Suggested fix: Change the bases to (TavilySearchVerifyRequest, BaseVerifyResponse, JudgeEvaluation) — or drop reward from JudgeEvaluation and inherit it — so every /verify response is nominally a BaseVerifyResponse.

+++ Evidence

class TavilySearchVerifyResponse(TavilySearchVerifyRequest, JudgeEvaluation):  # JudgeEvaluation(BaseModel) supplies `reward: float` ; 67 other servers subclass BaseVerifyResponse directly

+++